Skip to content

Commit 161936f

Browse files
fix(inverter): choose charge/discharge rate path from this inverter's own entry (#5359)
get_current_charge_rate()/get_current_discharge_rate() picked the percent-vs-watts path from whether "charge_rate_percent"/"discharge_rate_percent" existed ANYWHERE in args, not from this inverter's own entry. On a mixed fleet where one inverter uses percent control and another uses watts, the watts-only inverter's unset percent slot quietly resolved to the 100% default, so it always read back battery_rate_max_raw regardless of what was last written - the #3311 fault for a script-driven inverter. adjust_charge_rate()/adjust_discharge_rate() had the same bug on the write side: both registers were written whenever either key existed anywhere, so the watts-only inverter tried to write its own (unset) percent entity every cycle, logging a spurious "No entity_id for ..." warning and pinning had_errors onto the status sensor. Also fixes create_missing_arg(): when a key was entirely absent from args, it filled all four per-inverter slots with the same default value rather than padding every slot but the caller's own with None (as the already-existing short-list branch does). On a mixed fleet that left a plain number in a sibling inverter's slot before that inverter was even constructed, which the per-entry fix above would otherwise misread as a configured entity. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent d6985b6 commit 161936f

2 files changed

Lines changed: 194 additions & 11 deletions

File tree

‎apps/predbat/inverter.py‎

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -315,7 +315,13 @@ def create_missing_arg(self, arg, default):
315315
Create a missing inverter argument which will be assigned to a dummy entity
316316
"""
317317
if (arg not in self.base.args) or (not isinstance(self.base.args[arg], list)):
318-
self.base.args[arg] = [default, default, default, default]
318+
# Padded with None rather than the default, for the same reason as the short-list branch
319+
# below: the caller overwrites [self.id] with its dummy entity straight afterwards, so a
320+
# bare default left in another inverter's slot reads back as "configured" there too. On a
321+
# mixed fleet that turns a plain number into a phantom per-entry entity for a sibling
322+
# inverter that never asked for one, which the per-entry rate-rate path then treats as
323+
# set (#5359)
324+
self.base.args[arg] = [default if i == self.id else None for i in range(4)]
319325
elif len(self.base.args[arg]) <= self.id:
320326
# A list that stops short of this inverter is just as missing for it as no list at all,
321327
# and every caller assigns into [self.id] straight afterwards - so a short one raised
@@ -2115,7 +2121,10 @@ def get_current_discharge_rate(self):
21152121
Get the current discharge rate in watts
21162122
"""
21172123

2118-
if "discharge_rate_percent" in self.base.args:
2124+
# Pick the percent vs watts path from this inverter's own entry, not from whether the key
2125+
# exists anywhere in args - on a mixed fleet another inverter can set the key while this
2126+
# one's own slot is empty, which used to fall through to the percent default of 100% (#5359)
2127+
if self.base.get_arg("discharge_rate_percent", indirect=False, index=self.id):
21192128
current_rate = int(self.base.get_arg("discharge_rate_percent", index=self.id, default=100.0, required_unit="%") * self.battery_rate_max_raw / 100)
21202129
else:
21212130
current_rate = self.base.get_arg("discharge_rate", index=self.id, default=self.battery_rate_max_raw, required_unit="W")
@@ -2132,7 +2141,8 @@ def get_current_charge_rate(self):
21322141
"""
21332142
Get the current charge rate in watts
21342143
"""
2135-
if "charge_rate_percent" in self.base.args:
2144+
# See get_current_discharge_rate() above - same per-entry rule (#5359)
2145+
if self.base.get_arg("charge_rate_percent", indirect=False, index=self.id):
21362146
current_rate = self.base.get_arg("charge_rate_percent", index=self.id, default=100.0, required_unit="%") * self.battery_rate_max_raw / 100
21372147
else:
21382148
current_rate = self.base.get_arg("charge_rate", index=self.id, default=self.battery_rate_max_raw, required_unit="W")
@@ -2195,17 +2205,22 @@ def adjust_charge_rate(self, new_rate, notify=True):
21952205
fuzzy, fuzzy_below = self.rate_tolerances(self.battery_rate_max_charge)
21962206
if not within_fuzzy(current_rate, new_rate, fuzzy, fuzzy_below):
21972207
self.base.log("Inverter {} current charge rate is {}W and new target is {}W".format(self.id, current_rate, new_rate))
2198-
if "charge_rate" in self.base.args:
2208+
# Write only the register(s) this inverter's own entry actually configures - checking
2209+
# "key in args" writes both registers on a mixed fleet whenever ANY inverter sets the
2210+
# other one, which hits an empty entry and logs a spurious "No entity_id" error (#5359)
2211+
charge_rate_entity = self.base.get_arg("charge_rate", indirect=False, index=self.id, required_unit="W")
2212+
if charge_rate_entity:
21992213
self.write_and_poll_value(
22002214
"charge_rate",
2201-
self.base.get_arg("charge_rate", indirect=False, index=self.id, required_unit="W"),
2215+
charge_rate_entity,
22022216
new_rate,
22032217
fuzzy=fuzzy,
22042218
required_unit="W",
22052219
fuzzy_below=fuzzy_below,
22062220
)
2207-
if "charge_rate_percent" in self.base.args:
2208-
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="%")
2221+
charge_rate_percent_entity = self.base.get_arg("charge_rate_percent", indirect=False, index=self.id, required_unit="%")
2222+
if charge_rate_percent_entity:
2223+
self.write_and_poll_value("charge_rate_percent", charge_rate_percent_entity, min(int(new_rate / self.battery_rate_max_raw * 100), 100), fuzzy=5, required_unit="%")
22092224

22102225
if notify and self.base.set_inverter_notify:
22112226
self.base.call_notify(f"{self.base.prefix.capitalize()}: Inverter {self.id} charge rate changes to {new_rate}W at {self.base.time_now_str()}")
@@ -2244,17 +2259,21 @@ def adjust_discharge_rate(self, new_rate, notify=True):
22442259
fuzzy, fuzzy_below = self.rate_tolerances(self.battery_rate_max_discharge)
22452260
if not within_fuzzy(current_rate, new_rate, fuzzy, fuzzy_below):
22462261
self.base.log("Inverter {} current discharge rate is {}W and new target is {}W".format(self.id, current_rate, new_rate))
2247-
if "discharge_rate" in self.base.args:
2262+
# See adjust_charge_rate() above - write only the register(s) this inverter's own entry
2263+
# actually configures, not whichever happen to be set anywhere in the fleet (#5359)
2264+
discharge_rate_entity = self.base.get_arg("discharge_rate", indirect=False, index=self.id)
2265+
if discharge_rate_entity:
22482266
self.write_and_poll_value(
22492267
"discharge_rate",
2250-
self.base.get_arg("discharge_rate", indirect=False, index=self.id),
2268+
discharge_rate_entity,
22512269
new_rate,
22522270
fuzzy=fuzzy,
22532271
required_unit="W",
22542272
fuzzy_below=fuzzy_below,
22552273
)
2256-
if "discharge_rate_percent" in self.base.args:
2257-
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="%")
2274+
discharge_rate_percent_entity = self.base.get_arg("discharge_rate_percent", indirect=False, index=self.id, required_unit="%")
2275+
if discharge_rate_percent_entity:
2276+
self.write_and_poll_value("discharge_rate_percent", discharge_rate_percent_entity, min(int(new_rate / self.battery_rate_max_raw * 100), 100), fuzzy=5, required_unit="%")
22582277

22592278
if notify and self.base.set_inverter_notify:
22602279
self.base.call_notify(f"{self.base.prefix.capitalize()}: Inverter {self.id} discharge rate changes to {new_rate}W at {self.base.time_now_str()}")

‎apps/predbat/tests/test_inverter.py‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1079,6 +1079,158 @@ def quantising_call_service(service, **kwargs):
10791079
return failed
10801080

10811081

1082+
def test_mixed_fleet_rate_path_from_own_entry(test_name, ha, inv, prev_rate, percent_entry_set, discharge=False):
1083+
"""
1084+
On a mixed fleet, the percent-vs-watts path is chosen from THIS inverter's own entry, not from
1085+
whether the key exists anywhere in args (#5359).
1086+
1087+
Before the fix, get_current_charge_rate()/get_current_discharge_rate() took the percent branch
1088+
whenever "charge_rate_percent"/"discharge_rate_percent" was in self.base.args at all - so an
1089+
inverter whose own slot in that list is None (another inverter in the fleet set the key, this
1090+
one didn't) still took the percent branch, found nothing for its index, fell back to the 100%
1091+
default and always read back battery_rate_max_raw regardless of what was last written. With the
1092+
fix, an unset entry falls back to the watts-based "charge_rate"/"discharge_rate" entity instead.
1093+
"""
1094+
failed = False
1095+
print("Test: {} prev_rate {} percent_entry_set {} discharge {}".format(test_name, prev_rate, percent_entry_set, discharge))
1096+
1097+
percent_arg = "discharge_rate_percent" if discharge else "charge_rate_percent"
1098+
watt_entity = "number.discharge_rate" if discharge else "number.charge_rate"
1099+
percent_entity = "number.discharge_rate_percent" if discharge else "number.charge_rate_percent"
1100+
1101+
saved_percent_arg = inv.base.args.get(percent_arg)
1102+
try:
1103+
# Simulate a mixed fleet: the percent key exists in args (another inverter set it up) but
1104+
# this inverter's own index is unset.
1105+
inv.base.args[percent_arg] = percent_entity if percent_entry_set else None
1106+
ha.dummy_items[watt_entity] = prev_rate
1107+
ha.dummy_items[percent_entity] = 100 # Would read back as rate_max if the percent path were wrongly taken
1108+
1109+
current_rate = inv.get_current_discharge_rate() if discharge else inv.get_current_charge_rate()
1110+
finally:
1111+
if saved_percent_arg is not None:
1112+
inv.base.args[percent_arg] = saved_percent_arg
1113+
else:
1114+
inv.base.args.pop(percent_arg, None)
1115+
1116+
if percent_entry_set:
1117+
# This inverter's own entry IS set - percent path, read against the percent entity's 100%
1118+
expect_rate = inv.battery_rate_max_raw
1119+
else:
1120+
# This inverter's own entry is unset - falls back to the watts entity, which holds prev_rate
1121+
expect_rate = prev_rate
1122+
1123+
if current_rate != expect_rate:
1124+
print("ERROR: {} expected current rate {} got {}".format(test_name, expect_rate, current_rate))
1125+
failed = True
1126+
1127+
return failed
1128+
1129+
1130+
def test_mixed_fleet_rate_write_skips_unset_entry(test_name, ha, inv, discharge=False):
1131+
"""
1132+
adjust_charge_rate()/adjust_discharge_rate() must not write (or warn about) a register this
1133+
inverter's own entry does not configure, even when the sibling register's key exists in args
1134+
because another inverter in the fleet uses it (#5359).
1135+
1136+
Before the fix, both adjust functions wrote both "charge_rate"/"discharge_rate" and
1137+
"charge_rate_percent"/"discharge_rate_percent" whenever either key existed ANYWHERE in args,
1138+
regardless of this inverter's own entry - so a script-driven inverter sharing a fleet with a
1139+
percent-controlled one tried to write into its own (unset) percent entry every cycle, which
1140+
write_and_poll_value()'s check_write_entity() logs as "No entity_id for ..." and also pins
1141+
had_errors=True onto the status sensor.
1142+
"""
1143+
failed = False
1144+
print("Test: {} discharge {}".format(test_name, discharge))
1145+
1146+
percent_arg = "discharge_rate_percent" if discharge else "charge_rate_percent"
1147+
watt_arg = "discharge_rate" if discharge else "charge_rate"
1148+
watt_entity = "number.discharge_rate" if discharge else "number.charge_rate"
1149+
1150+
saved_percent_arg = inv.base.args.get(percent_arg)
1151+
saved_watt_arg = inv.base.args.get(watt_arg)
1152+
log_messages = []
1153+
orig_log = inv.base.log
1154+
1155+
try:
1156+
# This inverter is watts-only: its own charge_rate/discharge_rate entity is configured, but
1157+
# the percent key exists in args only because a sibling inverter in the fleet set it up -
1158+
# this inverter's own slot in that list is None.
1159+
inv.base.args[watt_arg] = watt_entity
1160+
inv.base.args[percent_arg] = None
1161+
inv.rest_data = None
1162+
inv.rest_api = None
1163+
ha.dummy_items[watt_entity] = 0
1164+
inv.base.log = lambda msg, *args, **kwargs: log_messages.append(str(msg))
1165+
1166+
if discharge:
1167+
inv.adjust_discharge_rate(250)
1168+
else:
1169+
inv.adjust_charge_rate(250)
1170+
finally:
1171+
inv.base.log = orig_log
1172+
if saved_percent_arg is not None:
1173+
inv.base.args[percent_arg] = saved_percent_arg
1174+
else:
1175+
inv.base.args.pop(percent_arg, None)
1176+
if saved_watt_arg is not None:
1177+
inv.base.args[watt_arg] = saved_watt_arg
1178+
else:
1179+
inv.base.args.pop(watt_arg, None)
1180+
1181+
warning_found = any("No entity_id for" in msg for msg in log_messages)
1182+
if warning_found:
1183+
print("ERROR: {} wrote/warned about the unset percent entry: {}".format(test_name, log_messages))
1184+
failed = True
1185+
if float(ha.get_state(watt_entity)) != 250:
1186+
print("ERROR: {} expected watt entity to be written to 250, got {}".format(test_name, ha.get_state(watt_entity)))
1187+
failed = True
1188+
1189+
return failed
1190+
1191+
1192+
def test_create_missing_arg_does_not_fill_sibling_entries(test_name, my_predbat):
1193+
"""
1194+
create_missing_arg() pads every OTHER inverter's slot with None, not the default (#5359 follow-up).
1195+
1196+
When the key is entirely missing from args, create_missing_arg() used to fill all four slots
1197+
with the same default value, and only the caller's own index was then overwritten with its
1198+
dummy entity. On a mixed fleet where inverter 0 triggers this (e.g. inv_output_charge_control
1199+
!= "power") before inverter 1 is even constructed, inverter 1's slot ended up holding a bare
1200+
number rather than None - which the per-entry "is this inverter's entry set" check added for
1201+
#5359 then misreads as a configured entity, since a truthy value is indistinguishable from one.
1202+
"""
1203+
failed = False
1204+
print(f"**** Running Test: {test_name} ****")
1205+
1206+
saved_args = copy.deepcopy(my_predbat.args)
1207+
try:
1208+
my_predbat.args["num_inverters"] = 2
1209+
# GS (Solis) drives charge rate via current rather than power, so constructing inverter 0
1210+
# takes the create_missing_arg("charge_rate", ...) branch under test
1211+
my_predbat.args["inverter_type"] = ["GS", "GS"]
1212+
my_predbat.args.pop("charge_rate", None)
1213+
my_predbat.args.pop("charge_rate_percent", None)
1214+
1215+
inv0 = Inverter(my_predbat, 0, quiet=True)
1216+
if inv0.inv_output_charge_control == "power":
1217+
print("ERROR: {} test assumes a non-power charge control to exercise create_missing_arg('charge_rate', ...)".format(test_name))
1218+
return 1
1219+
1220+
charge_rate_list = my_predbat.args.get("charge_rate")
1221+
if not isinstance(charge_rate_list, list) or len(charge_rate_list) < 2:
1222+
print("ERROR: {} expected charge_rate to be a per-inverter list covering inverter 1, got {}".format(test_name, charge_rate_list))
1223+
failed = True
1224+
elif charge_rate_list[1] is not None:
1225+
print("ERROR: {} expected inverter 1's charge_rate slot to stay None until inverter 1 is constructed, got {}".format(test_name, charge_rate_list[1]))
1226+
failed = True
1227+
finally:
1228+
my_predbat.args.clear()
1229+
my_predbat.args.update(saved_args)
1230+
1231+
return 1 if failed else 0
1232+
1233+
10821234
def test_custom_inverter_def_drops_rate_step(test_name, my_predbat, ha):
10831235
"""
10841236
A custom inverter type is built from a copy of the GE row, but must not inherit GivEnergy's rate step (#5324).
@@ -5504,6 +5656,17 @@ def run_inverter_tests(my_predbat_dummy):
55045656
if failed:
55055657
return failed
55065658

5659+
# #5359: mixed-fleet rate path must be chosen from THIS inverter's own entry, not from
5660+
# whether the percent key exists anywhere in args
5661+
failed |= test_mixed_fleet_rate_path_from_own_entry("mixed_fleet_charge_rate_entry_set", ha, inv, 200, percent_entry_set=True)
5662+
failed |= test_mixed_fleet_rate_path_from_own_entry("mixed_fleet_charge_rate_entry_unset", ha, inv, 200, percent_entry_set=False)
5663+
failed |= test_mixed_fleet_rate_path_from_own_entry("mixed_fleet_discharge_rate_entry_set", ha, inv, 250, percent_entry_set=True, discharge=True)
5664+
failed |= test_mixed_fleet_rate_path_from_own_entry("mixed_fleet_discharge_rate_entry_unset", ha, inv, 250, percent_entry_set=False, discharge=True)
5665+
failed |= test_mixed_fleet_rate_write_skips_unset_entry("mixed_fleet_charge_rate_write_skips_unset_percent", ha, inv)
5666+
failed |= test_mixed_fleet_rate_write_skips_unset_entry("mixed_fleet_discharge_rate_write_skips_unset_percent", ha, inv, discharge=True)
5667+
if failed:
5668+
return failed
5669+
55075670
# #4415: timed_charge_current/timed_discharge_current must be re-asserted every call, not
55085671
# just when the rate itself changes
55095672
failed |= test_current_reasserted_on_unchanged_rate("current_reassert_charge", ha, inv, 0, 200)
@@ -5985,6 +6148,7 @@ def run_inverter_tests(my_predbat_dummy):
59856148
failed |= test_export_window_no_source_configured_raises("export_window_no_source_configured_raises", my_predbat, dummy_items)
59866149
failed |= test_window_warning_names_components_when_type_unset("window_warning_names_components_when_type_unset", my_predbat, dummy_items)
59876150
failed |= test_short_per_inverter_list_gets_its_dummy_entity("short_per_inverter_list_gets_its_dummy_entity", my_predbat)
6151+
failed |= test_create_missing_arg_does_not_fill_sibling_entries("create_missing_arg_does_not_fill_sibling_entries", my_predbat)
59886152
if failed:
59896153
return failed
59906154

0 commit comments

Comments
 (0)