fix(givtcp): accept GivEnergy's quantised charge/discharge rate read-back (#5324)

apps/predbat/config.py

@@ -1852,6 +1852,7 @@
         "can_span_midnight": True,
         "charge_discharge_with_rate": False,
         "target_soc_used_for_discharge": False,
+        "rate_step_percent_of_capacity": 1,
     },
     "GEC": {
         "name": "GivEnergy Cloud",

apps/predbat/coordinator.py

@@ -100,6 +100,13 @@ def _clean_number(value):
     return None
 
 
+def _clean_percent(value):
+    """A percentage above 0 and up to 100, or None. Bools are refused, unlike _clean_number."""
+    if isinstance(value, bool) or not isinstance(value, (int, float)):
+        return None
+    return value if 0 < value <= 100 else None
+
+
 def _clean_token(value):
     """A vocabulary token (lower case, no spaces), or None."""
     return value if isinstance(value, str) and VOCAB_RE.match(value) else None
@@ -159,6 +166,7 @@ def _clean_measure_or_tokens(value):
     "min": _clean_number,
     "max": _clean_number,
     "step": _clean_number,
+    "step_percent_of_capacity": _clean_percent,
     "precision": _clean_number,
     "options": _clean_option_list,
     "format": _clean_string,
@@ -1319,6 +1327,12 @@ def not_applicable_field(field):
     else:
         gaps.append("output_charge_control")
         not_applicable_field("current_dp")
+    # charge_rate's step stands for both rates - an inverter holding them coarsely holds both alike. Most
+    # inverters have no such step, so its absence is not a gap: base's value stands, else none
+    if rate is not None and rate.get("step_percent_of_capacity"):
+        definition["rate_step_percent_of_capacity"] = rate["step_percent_of_capacity"]
+    else:
+        definition.setdefault("rate_step_percent_of_capacity", 0)
 
     definition["time_button_press"] = _bound_entity(entities, "schedule_write_button", access="rw") is not None
 

apps/predbat/gateway.py

@@ -100,6 +100,11 @@ def _serial_suffix(serial):
 PLAN_MODE_CHARGE = 1
 PLAN_MODE_DISCHARGE = 2
 
+# GivEnergy holds the charge and discharge rates as a whole percent of nominal battery capacity,
+# rounded down - 1300W on a 13.41kWh battery reads back 1206W (#5324)
+GIVENERGY_RATE_STEP_PERCENT_OF_CAPACITY = 1
+GIVENERGY_INVERTER_TYPES = (pb.INVERTER_TYPE_GIVENERGY, pb.INVERTER_TYPE_GIVENERGY_EMS, pb.INVERTER_TYPE_GIVENERGY_GATEWAY)
+
 # Entity attribute table — keyed by the semantic suffix used in dashboard_item calls
 GATEWAY_ATTRIBUTE_TABLE = {
     # Binary sensors
@@ -979,8 +984,15 @@ def _inject_inverter_entities(self, inv, suffix):
         _raw_export_limit = control.export_limit_w
         export_limit_publish = 99999 if _raw_export_limit == 0 else (0 if _raw_export_limit == 1 else _raw_export_limit)
         self.dashboard_item(f"sensor.{pfx}_export_limit_w", export_limit_publish, attributes=GATEWAY_ATTRIBUTE_TABLE.get("export_limit_w", {}), app="gateway")
-        self.dashboard_item(f"number.{pfx}_charge_rate", control.charge_rate_w, attributes=GATEWAY_ATTRIBUTE_TABLE.get("charge_rate", {}), app="gateway")
-        self.dashboard_item(f"number.{pfx}_discharge_rate", control.discharge_rate_w, attributes=GATEWAY_ATTRIBUTE_TABLE.get("discharge_rate", {}), app="gateway")
+        # The hub reports the rate the inverter holds, so a GivEnergy one reads back short of the rate written
+        # (#5324). Marked per inverter, on a copy of the table entry: GWMQTT is one type for every brand
+        charge_rate_attributes = dict(GATEWAY_ATTRIBUTE_TABLE.get("charge_rate", {}))
+        discharge_rate_attributes = dict(GATEWAY_ATTRIBUTE_TABLE.get("discharge_rate", {}))
+        if inv.type in GIVENERGY_INVERTER_TYPES:
+            charge_rate_attributes["step_percent_of_capacity"] = GIVENERGY_RATE_STEP_PERCENT_OF_CAPACITY
+            discharge_rate_attributes["step_percent_of_capacity"] = GIVENERGY_RATE_STEP_PERCENT_OF_CAPACITY
+        self.dashboard_item(f"number.{pfx}_charge_rate", control.charge_rate_w, attributes=charge_rate_attributes, app="gateway")
+        self.dashboard_item(f"number.{pfx}_discharge_rate", control.discharge_rate_w, attributes=discharge_rate_attributes, app="gateway")
         # The reserve ceiling is per-inverter, so it overrides the table's 100: GivEnergy
         # firmware refuses a reserve of 100 and the gateway reports 98 for it (gateway
         # issue #346). adjust_reserve() honours this entity's "max" through

apps/predbat/givtcp.py

@@ -92,6 +92,10 @@
 # restart. Only endpoints not already being managed are probed, and only ever to add them.
 GIVTCP_REDISCOVER_SECONDS = 3600
 
+# GivEnergy holds the charge and discharge rates as a whole percent of nominal battery capacity,
+# rounded down - 1300W on a 13.41kWh battery reads back 1206W (#5324)
+GIVTCP_RATE_STEP_PERCENT_OF_CAPACITY = 1
+
 # control name -> (domain, GivTCPRest write method name, HA entity attributes)
 GIVTCP_CONTROLS = {
     "charge_rate": ("number", "set_charge_rate", {"unit_of_measurement": "W", "device_class": "power", "icon": "mdi:battery-charging", "min": 0, "max": 20000, "step": 100}),
@@ -1038,6 +1042,7 @@ def _discovery_descriptor(self, n, name, domain, access, attrs, max_battery_rate
         elif options:
             descriptor["options"] = options
         if name in ("charge_rate", "discharge_rate"):
+            descriptor["step_percent_of_capacity"] = GIVTCP_RATE_STEP_PERCENT_OF_CAPACITY
             if max_battery_rate:
                 descriptor["max"] = max_battery_rate
             else:

apps/predbat/inverter.py

@@ -55,6 +55,16 @@
 _NOT_COMMITTED = object()
 
 
+def within_fuzzy(value, target, fuzzy, fuzzy_below=None):
+    """
+    Whether a numeric read counts as target: within fuzzy either side, or within fuzzy_below when it is
+    short of target and fuzzy_below is given - for a value the device rounds down (#5324).
+    """
+    if fuzzy_below is not None and value < target:
+        return target - value <= fuzzy_below
+    return abs(value - target) <= fuzzy
+
+
 class Inverter:
     """Unified inverter control abstraction for multiple brands.
 
@@ -469,6 +479,8 @@ def refresh_config(self, quiet=False):
         if "inverter" in self.base.args:
             if self.inverter_type not in INVERTER_DEF:
                 INVERTER_DEF[self.inverter_type] = INVERTER_DEF["GE"].copy()
+                # GivEnergy's rate quantisation is a fact about its hardware, not a default for other inverters
+                INVERTER_DEF[self.inverter_type].pop("rate_step_percent_of_capacity", None)
 
             inverter_def = self.base.args["inverter"]
             if isinstance(inverter_def, list):
@@ -523,6 +535,17 @@ def refresh_config(self, quiet=False):
         self.inv_charge_discharge_with_rate = INVERTER_DEF[self.inverter_type].get("charge_discharge_with_rate", False)
         self.inv_target_soc_used_for_discharge = INVERTER_DEF[self.inverter_type].get("target_soc_used_for_discharge", True)
         self.inv_has_solis_energy_control = INVERTER_DEF[self.inverter_type].get("has_solis_energy_control", False)
+        # The charge_rate entity can carry the step itself, for a type that covers more than one brand: the
+        # hub is GWMQTT whatever it drives, and marks only its GivEnergy inverters. The row's value stands otherwise
+        self.inv_rate_step_percent_of_capacity = INVERTER_DEF[self.inverter_type].get("rate_step_percent_of_capacity", 0)
+        rate_entity = self.base.get_arg("charge_rate", indirect=False, index=self.id)
+        if rate_entity:
+            try:
+                rate_step = float(self.base.get_state_wrapper(rate_entity, attribute="step_percent_of_capacity"))
+            except (ValueError, TypeError):
+                rate_step = 0
+            if 0 < rate_step <= 100:
+                self.inv_rate_step_percent_of_capacity = rate_step
 
         # If it's not a GE inverter then turn Quiet off
         if self.inverter_type != "GE":
@@ -2078,6 +2101,30 @@ def get_current_charge_rate(self):
 
         return current_rate
 
+    def rate_tolerances(self, rate_max):
+        """
+        (fuzzy, fuzzy_below) in watts for checking a charge/discharge rate read-back against the rate written.
+
+        fuzzy is 5% of rate_max - the rate ceiling, battery_rate_max_charge/discharge per minute. On an
+        inverter that stores the rate coarsely, fuzzy_below widens a read-back SHORT of the rate to one
+        hardware step. GivEnergy holds it as a whole percent of nominal battery capacity, rounded down:
+        1300W on a 13.41kWh battery reads back 1206W, a 94W miss that a 1300W ceiling's 65W tolerance
+        rejected - re-writing it ten times every cycle (#5324). A read-back over the rate is never that
+        rounding, so it keeps fuzzy: a rate held one step up (134W) is still written down to a 0W hold.
+
+        Deliberately relaxed rather than predicting the exact read-back, which would need Predbat's
+        capacity to match the inverter's to the watt. The cost is a ~1W boundary: a short read-back can
+        miss by just under one step plus 1W of read-back truncation, so a request landing exactly one step
+        above the held rate is taken as already set and the rate stays one step low - the same error the
+        rounding makes anyway. The step is only as right as nominal_capacity; GivTCP supplies its own.
+        """
+        fuzzy = rate_max * MINUTE_WATT / 20
+        fuzzy_below = None
+        if self.inv_rate_step_percent_of_capacity and self.nominal_capacity:
+            step = self.nominal_capacity * 1000 * self.inv_rate_step_percent_of_capacity / 100
+            fuzzy_below = max(fuzzy, step + 1)
+        return fuzzy, fuzzy_below
+
     def adjust_charge_rate(self, new_rate, notify=True):
         """
         Adjust charging rate
@@ -2102,10 +2149,18 @@ def adjust_charge_rate(self, new_rate, notify=True):
         new_rate = int(new_rate + 0.5)
         current_rate = self.get_current_charge_rate()
 
-        if abs(current_rate - new_rate) > (self.battery_rate_max_charge * MINUTE_WATT / 20):
+        fuzzy, fuzzy_below = self.rate_tolerances(self.battery_rate_max_charge)
+        if not within_fuzzy(current_rate, new_rate, fuzzy, fuzzy_below):
             self.base.log("Inverter {} current charge rate is {}W and new target is {}W".format(self.id, current_rate, new_rate))
             if "charge_rate" in self.base.args:
-                self.write_and_poll_value("charge_rate", self.base.get_arg("charge_rate", indirect=False, index=self.id, required_unit="W"), new_rate, fuzzy=(self.battery_rate_max_charge * MINUTE_WATT / 20), required_unit="W")
+                self.write_and_poll_value(
+                    "charge_rate",
+                    self.base.get_arg("charge_rate", indirect=False, index=self.id, required_unit="W"),
+                    new_rate,
+                    fuzzy=fuzzy,
+                    required_unit="W",
+                    fuzzy_below=fuzzy_below,
+                )
             if "charge_rate_percent" in self.base.args:
                 self.write_and_poll_value("charge_rate_percent", self.base.get_arg("charge_rate_percent", indirect=False, index=self.id, required_unit="%"), min(int(new_rate / self.battery_rate_max_raw * 100), 100), fuzzy=5, required_unit="%")
 
@@ -2143,10 +2198,18 @@ def adjust_discharge_rate(self, new_rate, notify=True):
         new_rate = int(new_rate + 0.5)
         current_rate = self.get_current_discharge_rate()
 
-        if abs(current_rate - new_rate) > (self.battery_rate_max_discharge * MINUTE_WATT / 20):
+        fuzzy, fuzzy_below = self.rate_tolerances(self.battery_rate_max_discharge)
+        if not within_fuzzy(current_rate, new_rate, fuzzy, fuzzy_below):
             self.base.log("Inverter {} current discharge rate is {}W and new target is {}W".format(self.id, current_rate, new_rate))
             if "discharge_rate" in self.base.args:
-                self.write_and_poll_value("discharge_rate", self.base.get_arg("discharge_rate", indirect=False, index=self.id), new_rate, fuzzy=(self.battery_rate_max_discharge * MINUTE_WATT / 20), required_unit="W")
+                self.write_and_poll_value(
+                    "discharge_rate",
+                    self.base.get_arg("discharge_rate", indirect=False, index=self.id),
+                    new_rate,
+                    fuzzy=fuzzy,
+                    required_unit="W",
+                    fuzzy_below=fuzzy_below,
+                )
             if "discharge_rate_percent" in self.base.args:
                 self.write_and_poll_value("discharge_rate_percent", self.base.get_arg("discharge_rate_percent", indirect=False, index=self.id, required_unit="%"), min(int(new_rate / self.battery_rate_max_raw * 100), 100), fuzzy=5, required_unit="%")
 
@@ -2411,9 +2474,10 @@ def switch_matched(state):
             self._write_backoff_result(name, entity_id, new_value, False)
             return False
 
-    def write_and_poll_value(self, name, entity_id, new_value, fuzzy=0, ignore_fail=False, required_unit=None):
+    def write_and_poll_value(self, name, entity_id, new_value, fuzzy=0, ignore_fail=False, required_unit=None, fuzzy_below=None):
         # Modified to cope with sensor entities and writing strings
         # Re-written to minimise writes
+        # fuzzy_below, when given, replaces fuzzy for a read short of new_value - for a value the device rounds down
         if not self.check_write_entity("write_and_poll_value", name, entity_id, new_value):
             return False
         domain, entity_name = entity_id.split(".")
@@ -2443,7 +2507,7 @@ def value_matched(state):
             state = value_state(state)
             if isinstance(new_value, str):
                 return state == new_value
-            return abs(state - new_value) <= fuzzy
+            return within_fuzzy(state, new_value, fuzzy, fuzzy_below)
 
         raw_state = self.base.get_state_wrapper(entity_id, required_unit=required_unit)
         current_state = value_state(raw_state, warn=True)

apps/predbat/tests/discovery_contract.py

@@ -25,6 +25,7 @@
     "has_ge_eco_toggle": False,
     "charge_discharge_with_rate": False,
     "target_soc_used_for_discharge": True,
+    "rate_step_percent_of_capacity": 0,
 }
 
 # Row fields the record is not expected to rebuild: the display name, and two fields nothing reads

apps/predbat/tests/test_coordinator.py

@@ -1877,6 +1877,21 @@ def test_descriptor_invert_must_be_bool():
     return 0
 
 
+def test_descriptor_step_percent_of_capacity_must_be_a_percentage():
+    """step_percent_of_capacity is kept when it is a percentage above 0 and up to 100, dropped otherwise (#5324)."""
+    record, _ = _one_inverter(entities={
+        "charge_rate": {"entity_id": "number.c", "access": "rw", "step_percent_of_capacity": 1},
+        "discharge_rate": {"entity_id": "number.d", "access": "rw", "step_percent_of_capacity": True},
+        "charge_limit": {"entity_id": "number.l", "access": "rw", "step_percent_of_capacity": 0},
+        "reserve": {"entity_id": "number.r", "access": "rw", "step_percent_of_capacity": 150},
+    })
+    assert record["entities"]["charge_rate"]["step_percent_of_capacity"] == 1, record
+    for setting in ("discharge_rate", "charge_limit", "reserve"):
+        assert "step_percent_of_capacity" not in record["entities"][setting], record
+    print("PASS: step_percent_of_capacity must be a percentage")
+    return 0
+
+
 def test_new_containers_survive_redaction_unchanged():
     """Bool capabilities, a strftime format and a string value stand-in pass the redactor untouched."""
     base, coordinator = _coordinator()
@@ -1947,6 +1962,23 @@ def test_inverter_definition_presence_needs_a_real_rw_entity():
     return 0
 
 
+def test_inverter_definition_rate_step_from_charge_rate():
+    """rate_step_percent_of_capacity comes from charge_rate's step_percent_of_capacity; absent, base's value stands, else 0 (#5324)."""
+    record = _full_record()
+    definition, gaps, _ = inverter_definition(record, 2)
+    assert definition["rate_step_percent_of_capacity"] == 0 and "rate_step_percent_of_capacity" not in gaps, definition
+    definition, _, _ = inverter_definition(record, 2, base={"rate_step_percent_of_capacity": 1})
+    assert definition["rate_step_percent_of_capacity"] == 1, definition
+    record["entities"]["charge_rate"]["step_percent_of_capacity"] = 1
+    definition, _, _ = inverter_definition(record, 2)
+    assert definition["rate_step_percent_of_capacity"] == 1, definition
+    del record["entities"]["charge_rate"]
+    definition, _, _ = inverter_definition(record, 2)
+    assert definition["rate_step_percent_of_capacity"] == 0, definition
+    print("PASS: rate step from charge_rate")
+    return 0
+
+
 def test_inverter_definition_ge_mode_flags_follow_inverter_mode_domain():
     """inverter_mode as a select means a GE inverter-mode select; as a switch, GE Cloud's eco toggle."""
     record = _full_record()
@@ -2119,10 +2151,12 @@ def test_coordinator_all(my_predbat=None):
     failures += test_descriptor_needs_exactly_one_of_entity_id_and_value()
     failures += test_descriptor_value_rejects_free_text()
     failures += test_descriptor_invert_must_be_bool()
+    failures += test_descriptor_step_percent_of_capacity_must_be_a_percentage()
     failures += test_new_containers_survive_redaction_unchanged()
     failures += test_inverter_definition_builds_every_field_without_a_base()
     failures += test_inverter_definition_presence_needs_a_real_rw_entity()
     failures += test_inverter_definition_ge_mode_flags_follow_inverter_mode_domain()
+    failures += test_inverter_definition_rate_step_from_charge_rate()
     failures += test_inverter_definition_charge_rate_units()
     failures += test_inverter_definition_gaps_and_not_applicable()
     failures += test_inverter_definition_counts_load_entities()

apps/predbat/tests/test_gateway.py

@@ -666,6 +666,42 @@ def test_reserve_soc_max_does_not_mutate_the_shared_table(self):
 
         assert GATEWAY_ATTRIBUTE_TABLE["reserve_soc"]["max"] == 100
 
+    def test_rate_step_marked_on_givenergy_rates(self):
+        """A GivEnergy inverter's rate entities carry the 1% of capacity step its read-back is rounded down to (#5324)."""
+        status = self._make_status()
+        for inverter_type in (pb.INVERTER_TYPE_GIVENERGY, pb.INVERTER_TYPE_GIVENERGY_EMS, pb.INVERTER_TYPE_GIVENERGY_GATEWAY):
+            status.inverters[0].type = inverter_type
+            gw = self._make_gateway()
+            gw._inject_entities(status)
+
+            for rate in ("charge_rate", "discharge_rate"):
+                state, attrs = gw._dashboard_calls["number.predbat_gateway_456789_" + rate]
+                assert state == 3000
+                assert attrs["step_percent_of_capacity"] == 1, (inverter_type, rate, attrs)
+                # The rest of the table entry survives
+                assert attrs["unit_of_measurement"] == "W"
+                assert attrs["step"] == 10
+
+    def test_rate_step_not_marked_on_other_brands(self):
+        """GWMQTT is one inverter type for every brand the hub drives, so the GivEnergy step must not reach the others."""
+        from gateway import GATEWAY_ATTRIBUTE_TABLE
+
+        status = self._make_status()
+        for inverter_type in (pb.INVERTER_TYPE_SOLIS_HYBRID, pb.INVERTER_TYPE_DEYE_SUNSYNK, pb.INVERTER_TYPE_UNKNOWN):
+            status.inverters[0].type = inverter_type
+            gw = self._make_gateway()
+            gw._inject_entities(status)
+
+            for rate in ("charge_rate", "discharge_rate"):
+                _, attrs = gw._dashboard_calls["number.predbat_gateway_456789_" + rate]
+                assert "step_percent_of_capacity" not in attrs, (inverter_type, rate, attrs)
+
+        # Marked on a copy: a GivEnergy inverter must not leave the step in the shared table
+        status.inverters[0].type = pb.INVERTER_TYPE_GIVENERGY
+        self._make_gateway()._inject_entities(status)
+        for rate in ("charge_rate", "discharge_rate"):
+            assert "step_percent_of_capacity" not in GATEWAY_ATTRIBUTE_TABLE[rate]
+
     def test_ems_aggregate_entities(self):
         """EMS aggregate and sub-inverter entities are published with table attributes."""
         from gateway import GATEWAY_ATTRIBUTE_TABLE