fix(solis): guard battery_scaling against a documented 0% SOH API response (#4500)

* fix(solis): guard battery_scaling against a documented 0% SOH API response (#4494)

A Solis Cloud API batteryHealthSoh of 0 is a documented, valid response (not
"no battery"), but was previously treated as falsy in automatic_config()'s
battery detection and, once bound to the live SOH sensor, fed straight into
soc_max = nominal_capacity * battery_scaling - collapsing soc_max to 0 and
making best_soc_keep permanently unreachable, forcing continuous grid
charging regardless of price. Reported with full root-cause trace by
@jibbej.

- inverter.py: a battery_scaling reading of 0 or negative is ambiguous
  (flaky API vs a genuinely unhealthy battery), so rather than asserting
  either extreme, retain the last value that was actually read as valid
  (mirrors the existing soc_max_nominal fallback pattern), falling back to
  1.0 only if nothing valid has ever been read.
- solis.py automatic_config(): a battery_soh of 0 no longer excludes the
  inverter from configuration the same way a genuinely missing field does,
  which previously caused automatic_config() to abandon configuration
  entirely (load_today/charge_start_time left unset) and crash-loop.

* feat(components): warn once when auto-discovery overrides an apps.yaml value (#4494 follow-up)

Reviewer feedback on PR #4500 (jibbej): battery_scaling being silently
discarded was fixed, but the underlying discoverability problem is
generic - automatic_config() (in any cloud integration) always wins over
an explicit apps.yaml value via set_arg(), with zero indication to the
user that their setting was ignored.

Adds ComponentBase.set_arg_auto(), a drop-in replacement for set_arg()
intended for auto-discovery code: if the key had a different value in
the user's raw apps.yaml (snapshotted at the very start of
PredBat.initialize(), before Predbat's own defaulting or any component
touches self.args), logs a one-time note naming both values, then applies
the auto-discovered value exactly as before - precedence is unchanged,
only the visibility of the override is new.

Wired up for all of solis.py's automatic_config() bindings, since the
same silent-override behaviour applies equally to every key it touches,
not just battery_scaling. Other integrations are untouched for now - the
helper lives on the shared ComponentBase so adopting it elsewhere is a
follow-up, not a redesign.

Author: chalfontchubby

apps/predbat/component_base.py

@@ -93,6 +93,22 @@ def set_arg(self, arg, value):
         """
         return self.base.set_arg(arg, value)
 
+    def set_arg_auto(self, arg, value):
+        """
+        Like set_arg(), but for auto-discovery code (typically automatic_config()) binding an
+        apps.yaml key to an auto-discovered entity/value. Auto-discovery still always wins - this
+        does not change that - but if the user had already set this key explicitly in apps.yaml,
+        silently discarding it left no way to notice (issue #4494 follow-up discussion, PR #4500).
+        Logs a one-time note per key when that happens, then behaves exactly like set_arg().
+        """
+        raw_args = getattr(self.base, "args_from_apps_yaml", None) or {}
+        raw_value = raw_args.get(arg)
+        warned = getattr(self.base, "apps_yaml_override_warned", None)
+        if raw_value is not None and raw_value != value and warned is not None and arg not in warned:
+            warned.add(arg)
+            self.log(f"Note: apps.yaml sets '{arg}: {raw_value}' but auto-discovery is using '{value}' instead - auto-discovery always wins currently; remove the apps.yaml entry to avoid this message")
+        return self.set_arg(arg, value)
+
     def get_arg(self, arg, default=None, indirect=True, combine=False, attribute=None, index=None, domain=None, can_override=True, required_unit=None):
         """
         Retrieve a configuration argument from the base system.

apps/predbat/inverter.py

@@ -154,6 +154,18 @@ def __init__(self, base, id=0, quiet=False, rest_postCommand=None, rest_getData=
         self.reserve_percent = self.base.get_arg("battery_min_soc", default=4.0, index=self.id, required_unit="%")
         self.reserve_percent_current = self.base.get_arg("battery_min_soc", default=4.0, index=self.id, required_unit="%")
         self.battery_scaling = self.base.get_arg("battery_scaling", default=1.0, index=self.id)
+        if not self.battery_scaling or self.battery_scaling <= 0:
+            # A parsed value of exactly 0 (or negative) isn't safe to interpret either way - it could
+            # be a flaky/unavailable API response (e.g. Solis Cloud API returning batteryHealthSoh: 0
+            # during an outage) or a genuinely unhealthy battery, and asserting either "fully healthy"
+            # (1.0) or "no capacity" (0) would be guessing. Retain the last value that was actually
+            # read as valid, rather than inventing one; only fall back to 1.0 if nothing valid has
+            # ever been read for this inverter.
+            last_known = self.base.get_arg("battery_scaling_last_known", default=1.0, index=self.id)
+            self.log("Warn: Inverter {} battery_scaling read as {} which is not a valid scaling factor, retaining last known value {} for this cycle".format(self.id, self.battery_scaling, last_known))
+            self.battery_scaling = last_known
+        else:
+            self.base.set_arg("battery_scaling_last_known", self.battery_scaling, index=self.id)
         self.battery_scaling_config = self.battery_scaling
 
         self.reserve_max = 100

apps/predbat/predbat.py

@@ -1603,6 +1603,12 @@ def initialize(self):
         """
         Setup the app, called once each time the app starts
         """
+        # Snapshot of apps.yaml exactly as the user wrote it, before Predbat's own defaulting
+        # (auto_config/load_user_config) or any component's automatic_config() touches self.args -
+        # lets ComponentBase.set_arg_auto() tell "user explicitly configured this" apart from
+        # "Predbat defaulted it" or "another component already overwrote it" (issue #4494 follow-up).
+        self.args_from_apps_yaml = copy.deepcopy(self.args)
+        self.apps_yaml_override_warned = set()  # {arg} already warned about via set_arg_auto()
         self.pool = None
         self.log("Predbat: Startup {}".format(__name__))
         self.update_time(print=False)

apps/predbat/solis.py

@@ -1241,7 +1241,9 @@ async def automatic_config(self):
                 battery_soh = float(battery_soh) / 100.0
             except (ValueError, TypeError):
                 battery_soh = None
-            if battery_soh:
+            # A battery_soh of exactly 0 is a documented, valid Solis Cloud API response (not "no
+            # battery") - only a missing/unparseable field should exclude the inverter here.
+            if battery_soh is not None:
                 batteries.append(inverter_sn)
 
         num_inverters = len(batteries)
@@ -1254,50 +1256,50 @@ async def automatic_config(self):
         devices = [sn.lower() for sn in batteries]
 
         # Configure base Predbat settings
-        self.set_arg("inverter_type", ["SolisCloud" for _ in range(num_inverters)])
-        self.set_arg("num_inverters", num_inverters)
+        self.set_arg_auto("inverter_type", ["SolisCloud" for _ in range(num_inverters)])
+        self.set_arg_auto("num_inverters", num_inverters)
 
         # Battery and inverter entities
-        self.set_arg("soc_percent", [f"sensor.{self.prefix}_solis_{device}_battery_soc" for device in devices])
-        self.set_arg("battery_scaling", [f"sensor.{self.prefix}_solis_{device}_battery_soh" for device in devices])
-        self.set_arg("battery_power", [f"sensor.{self.prefix}_solis_{device}_battery_power" for device in devices])
-        self.set_arg("battery_power_invert", [f"True" for device in devices])
-        self.set_arg("grid_power", [f"sensor.{self.prefix}_solis_{device}_grid_power" for device in devices])
-        self.set_arg("battery_voltage", [f"sensor.{self.prefix}_solis_{device}_battery_voltage" for device in devices])
+        self.set_arg_auto("soc_percent", [f"sensor.{self.prefix}_solis_{device}_battery_soc" for device in devices])
+        self.set_arg_auto("battery_scaling", [f"sensor.{self.prefix}_solis_{device}_battery_soh" for device in devices])
+        self.set_arg_auto("battery_power", [f"sensor.{self.prefix}_solis_{device}_battery_power" for device in devices])
+        self.set_arg_auto("battery_power_invert", [f"True" for device in devices])
+        self.set_arg_auto("grid_power", [f"sensor.{self.prefix}_solis_{device}_grid_power" for device in devices])
+        self.set_arg_auto("battery_voltage", [f"sensor.{self.prefix}_solis_{device}_battery_voltage" for device in devices])
         # self.set_arg("battery_temperature", [f"sensor.{self.prefix}_solis_{device}_battery_temperature" for device in devices])
 
         # if solis_cloud_pv_load_ignore is set to true, override Solis cloud sensors and use load/pv_today/power entries defined in apps.yaml
         if not self.get_arg("solis_cloud_pv_load_ignore", default=False):
-            self.set_arg("load_today", [f"sensor.{self.prefix}_solis_{device}_total_load_energy" for device in devices])
-            self.set_arg("pv_today", [f"sensor.{self.prefix}_solis_{device}_pv_energy_total" for device in devices])
-            self.set_arg("load_power", [f"sensor.{self.prefix}_solis_{device}_load_power" for device in devices])
-            self.set_arg("pv_power", [f"sensor.{self.prefix}_solis_{device}_pv_power" for device in devices])
-        self.set_arg("import_today", [f"sensor.{self.prefix}_solis_{device}_today_import_energy" for device in devices])
-        self.set_arg("export_today", [f"sensor.{self.prefix}_solis_{device}_today_export_energy" for device in devices])
+            self.set_arg_auto("load_today", [f"sensor.{self.prefix}_solis_{device}_total_load_energy" for device in devices])
+            self.set_arg_auto("pv_today", [f"sensor.{self.prefix}_solis_{device}_pv_energy_total" for device in devices])
+            self.set_arg_auto("load_power", [f"sensor.{self.prefix}_solis_{device}_load_power" for device in devices])
+            self.set_arg_auto("pv_power", [f"sensor.{self.prefix}_solis_{device}_pv_power" for device in devices])
+        self.set_arg_auto("import_today", [f"sensor.{self.prefix}_solis_{device}_today_import_energy" for device in devices])
+        self.set_arg_auto("export_today", [f"sensor.{self.prefix}_solis_{device}_today_export_energy" for device in devices])
 
         # Battery capacity and limits from cached details
         # XXX: This is currently broken, user must set manually in apps.yaml
         # self.set_arg("soc_max", [f"sensor.{self.prefix}_solis_{device}_battery_capacity" for device in devices])
 
         # Reserve and limits
-        self.set_arg("reserve", [f"number.{self.prefix}_solis_{device}_over_discharge_soc" for device in devices])
-        self.set_arg("battery_min_soc", [f"number.{self.prefix}_solis_{device}_over_discharge_soc" for device in devices])
+        self.set_arg_auto("reserve", [f"number.{self.prefix}_solis_{device}_over_discharge_soc" for device in devices])
+        self.set_arg_auto("battery_min_soc", [f"number.{self.prefix}_solis_{device}_over_discharge_soc" for device in devices])
 
         # Charge/discharge controls - using slot 1 for Predbat primary control
-        self.set_arg("charge_start_time", [f"select.{self.prefix}_solis_{device}_charge_slot1_start_time" for device in devices])
-        self.set_arg("charge_end_time", [f"select.{self.prefix}_solis_{device}_charge_slot1_end_time" for device in devices])  # Same selector, parsed
-        self.set_arg("charge_limit", [f"number.{self.prefix}_solis_{device}_charge_slot1_soc" for device in devices])
-        self.set_arg("charge_rate", [f"number.{self.prefix}_solis_{device}_charge_slot1_power" for device in devices])
-        self.set_arg("scheduled_charge_enable", [f"switch.{self.prefix}_solis_{device}_charge_slot1_enable" for device in devices])
-
-        self.set_arg("discharge_start_time", [f"select.{self.prefix}_solis_{device}_discharge_slot1_start_time" for device in devices])
-        self.set_arg("discharge_end_time", [f"select.{self.prefix}_solis_{device}_discharge_slot1_end_time" for device in devices])
-        self.set_arg("discharge_target_soc", [f"number.{self.prefix}_solis_{device}_discharge_slot1_soc" for device in devices])
-        self.set_arg("discharge_rate", [f"number.{self.prefix}_solis_{device}_discharge_slot1_power" for device in devices])
-        self.set_arg("scheduled_discharge_enable", [f"switch.{self.prefix}_solis_{device}_discharge_slot1_enable" for device in devices])
-        self.set_arg("battery_rate_max", [f"number.{self.prefix}_solis_{device}_max_charge_power" for device in devices])
-        self.set_arg("inverter_limit", [f"sensor.{self.prefix}_solis_{device}_inverter_size" for device in devices])
-        self.set_arg("export_limit", [f"number.{self.prefix}_solis_{device}_max_export_power" for device in devices])
+        self.set_arg_auto("charge_start_time", [f"select.{self.prefix}_solis_{device}_charge_slot1_start_time" for device in devices])
+        self.set_arg_auto("charge_end_time", [f"select.{self.prefix}_solis_{device}_charge_slot1_end_time" for device in devices])  # Same selector, parsed
+        self.set_arg_auto("charge_limit", [f"number.{self.prefix}_solis_{device}_charge_slot1_soc" for device in devices])
+        self.set_arg_auto("charge_rate", [f"number.{self.prefix}_solis_{device}_charge_slot1_power" for device in devices])
+        self.set_arg_auto("scheduled_charge_enable", [f"switch.{self.prefix}_solis_{device}_charge_slot1_enable" for device in devices])
+
+        self.set_arg_auto("discharge_start_time", [f"select.{self.prefix}_solis_{device}_discharge_slot1_start_time" for device in devices])
+        self.set_arg_auto("discharge_end_time", [f"select.{self.prefix}_solis_{device}_discharge_slot1_end_time" for device in devices])
+        self.set_arg_auto("discharge_target_soc", [f"number.{self.prefix}_solis_{device}_discharge_slot1_soc" for device in devices])
+        self.set_arg_auto("discharge_rate", [f"number.{self.prefix}_solis_{device}_discharge_slot1_power" for device in devices])
+        self.set_arg_auto("scheduled_discharge_enable", [f"switch.{self.prefix}_solis_{device}_discharge_slot1_enable" for device in devices])
+        self.set_arg_auto("battery_rate_max", [f"number.{self.prefix}_solis_{device}_max_charge_power" for device in devices])
+        self.set_arg_auto("inverter_limit", [f"sensor.{self.prefix}_solis_{device}_inverter_size" for device in devices])
+        self.set_arg_auto("export_limit", [f"number.{self.prefix}_solis_{device}_max_export_power" for device in devices])
 
         self.log("Solis API: Automatic configuration complete")
 
@@ -1517,7 +1519,11 @@ async def publish_entities(self):
                 app="solis"
             )
 
-            # Battery state of health
+            # Battery state of health - published as-is, including a literal 0 (issue #4494): a 0%
+            # reading here can be a flaky/unavailable API response as well as a genuinely unhealthy
+            # battery, and we don't know which, so it's reported honestly rather than guessed at.
+            # Inverter.__init__ is where battery_scaling itself is protected from a 0 or negative
+            # reading (retains the last known-good value rather than collapsing soc_max).
             battery_soh = detail.get("batteryHealthSoh")
             try:
                 battery_soh = float(battery_soh) / 100.0

apps/predbat/tests/test_component_base.py

@@ -361,6 +361,50 @@ async def run_test():
     return asyncio.run(run_test())
 
 
+def test_component_base_set_arg_auto(my_predbat):
+    """
+    Test ComponentBase.set_arg_auto() (issue #4494 follow-up, PR #4500 review): warns once when
+    it overwrites a key the user had explicitly set in apps.yaml, otherwise behaves exactly like
+    set_arg() - auto-discovery always wins either way, this only makes the override discoverable.
+    """
+    print("\n*** Test: ComponentBase.set_arg_auto warns once on apps.yaml override ***")
+
+    base = MockBase()
+    base.args_from_apps_yaml = {"battery_scaling": [0.9]}
+    base.apps_yaml_override_warned = set()
+    set_calls = {}
+    base.set_arg = lambda arg, value: set_calls.__setitem__(arg, value)
+
+    component = TestComponent(base)
+
+    # apps.yaml had a different value - warn once, auto-discovered value still applied
+    component.set_arg_auto("battery_scaling", ["sensor.predbat_battery_soh"])
+    assert set_calls.get("battery_scaling") == ["sensor.predbat_battery_soh"], "Auto-discovered value should be applied"
+    assert any("apps.yaml sets 'battery_scaling: [0.9]'" in msg for msg in base.log_messages), "Should warn about the override"
+
+    # Second call for the same key must not repeat the warning
+    component.set_arg_auto("battery_scaling", ["sensor.predbat_battery_soh"])
+    warn_count = sum(1 for msg in base.log_messages if "apps.yaml sets 'battery_scaling" in msg)
+    assert warn_count == 1, f"Warning should not repeat, got {warn_count}"
+
+    # A key never present in apps.yaml at all - no warning, behaves like plain set_arg
+    component.set_arg_auto("num_inverters", 1)
+    assert set_calls.get("num_inverters") == 1, "Should still set the value for an unconfigured key"
+    assert not any("num_inverters" in msg for msg in base.log_messages), "Should not warn for a key the user never configured"
+
+    # Base with no args_from_apps_yaml snapshot at all (e.g. component created outside
+    # PredBat.initialize(), as in most unit tests) must not raise, and must not warn
+    bare_base = MockBase()
+    bare_set_calls = {}
+    bare_base.set_arg = lambda arg, value: bare_set_calls.__setitem__(arg, value)
+    bare_component = TestComponent(bare_base)
+    bare_component.set_arg_auto("battery_scaling", ["sensor.predbat_battery_soh"])
+    assert bare_set_calls.get("battery_scaling") == ["sensor.predbat_battery_soh"], "Should still work without an apps_yaml snapshot"
+
+    print("PASS: set_arg_auto warns once on a genuine override, stays silent otherwise, and is safe without a snapshot")
+    return False
+
+
 def test_component_base_all(my_predbat):
     """Run all component_base tests"""
     tests = [
@@ -372,6 +416,7 @@ def test_component_base_all(my_predbat):
         ("exception_handling", test_component_base_exception_handling, "Component handles exceptions with backoff"),
         ("run_timeout", test_component_base_run_timeout, "Hung run() triggers timeout, stack trace, and error count"),
         ("first_cleared_preset", test_component_base_first_cleared_when_run_presets_api_started, "first flag clears even when run() pre-sets api_started"),
+        ("set_arg_auto", test_component_base_set_arg_auto, "set_arg_auto warns once on an apps.yaml override, silent otherwise"),
     ]
 
     failed = []

apps/predbat/tests/test_inverter.py

@@ -2109,6 +2109,54 @@ def test_input_datetime_charge_window(test_name, ha, inv, dummy_rest, direction,
     return failed
 
 
+def test_battery_scaling_invalid_value_clamped(test_name, my_predbat):
+    """
+    Verify Inverter.__init__ guards against a zero or negative battery_scaling read from args
+    rather than letting it propagate into soc_max = nominal_capacity * battery_scaling.
+    A cloud API legitimately reporting 0% battery State of Health (e.g. Solis, issue #4494) must
+    not silently collapse soc_max to zero - and since 0 is ambiguous (could mean a flaky API
+    response or a genuinely unhealthy battery), Predbat must not guess a value either; it should
+    retain the last value that was actually read as valid, falling back to 1.0 only when nothing
+    valid has ever been read.
+    """
+    failed = False
+    print("**** Running Test: {} ****".format(test_name))
+
+    # A prior test in this run may have left givtcp_rest pointing at a dummy REST URL, which
+    # would otherwise make this plain construction attempt (and retry) a real REST read.
+    my_predbat.args["givtcp_rest"] = None
+    if "battery_scaling_last_known" in my_predbat.args:
+        del my_predbat.args["battery_scaling_last_known"]
+
+    # No prior valid reading exists yet - falls back to 1.0
+    for bad_scaling in (0.0, -0.5):
+        my_predbat.args["battery_scaling"] = [bad_scaling]
+        inv = Inverter(my_predbat, 0)
+        if inv.battery_scaling != 1.0:
+            print("ERROR: battery_scaling should fall back to 1.0 when source reads {} and no prior value exists, got {}".format(bad_scaling, inv.battery_scaling))
+            failed = True
+
+    # A valid reading passes through unchanged, and is remembered
+    my_predbat.args["battery_scaling"] = [0.72]
+    inv = Inverter(my_predbat, 0)
+    if inv.battery_scaling != 0.72:
+        print("ERROR: battery_scaling should pass a valid value through unchanged, got {}".format(inv.battery_scaling))
+        failed = True
+
+    # A subsequent invalid reading retains the last known-good value (0.72), not 1.0 - it must
+    # not be assumed the battery is now "fully healthy" just because the reading is unusable
+    for bad_scaling in (0.0, -0.5):
+        my_predbat.args["battery_scaling"] = [bad_scaling]
+        inv = Inverter(my_predbat, 0)
+        if inv.battery_scaling != 0.72:
+            print("ERROR: battery_scaling should retain last known-good value 0.72 when source reads {}, got {}".format(bad_scaling, inv.battery_scaling))
+            failed = True
+
+    del my_predbat.args["battery_scaling"]
+    del my_predbat.args["battery_scaling_last_known"]
+    return failed
+
+
 def test_rest_battery_capacity_fallback(test_name, my_predbat):
     """
     Verify that when V3 REST data omits Battery_Capacity_kWh and battery_nominal_capacity,
@@ -2454,6 +2502,8 @@ def run_inverter_tests(my_predbat_dummy):
     if failed:
         return failed
 
+    failed |= test_battery_scaling_invalid_value_clamped("battery_scaling_invalid_value_clamped", my_predbat)
+
     failed |= test_rest_battery_capacity_fallback("rest_capacity_fallback", my_predbat)
     if failed:
         return failed