Skip to content

Commit c84f711

Browse files
CIclaude
andcommitted
fix(solis): address #5247 review - lock the slot schedule, settle errored writes
- Slot events now change the schedule through set_slot_schedule_field(), under a threading.Lock that write_time_windows_if_changed() also holds while it deep-copies the schedule and that restore_refused_slot_write() holds for its check-and-update. Slot events arrive on another loop, so the Solis loop is no longer the only writer. None of the locked sections await. - Slot events are applied immediately only once the inverter's schedule has been decoded (schedules_loaded). The first run() discovers the inverter before it decodes, and the decode replaces the schedule wholesale, so an event applied in between was lost; until then it is queued. - read_and_write_cid() takes the settled re-read on the write-error path too. A value that lands a moment after a timed-out reply is a success, instead of being rolled back and then overwritten with the old value. - One route_event() helper replaces the three copies of the routing branch, and routing uses self.prefix instead of get_arg('prefix') on every event. - restore_refused_slot_write() documents why the whole start-end pair is restored, and the remaining window: a start event queued before the first decode. - The rollback tests now run the real read_and_write_cid() and read_cid() against a fake inverter with a stale cache. Removing read_cid()'s cache refresh makes them fail. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent 9407d4d commit c84f711

2 files changed

Lines changed: 225 additions & 108 deletions

File tree

‎apps/predbat/solis.py‎

Lines changed: 73 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import json
1515
import time
1616
import copy
17+
import threading
1718
from datetime import datetime, timedelta, UTC
1819
from predbat_metrics import record_api_call
1920
from component_base import ComponentBase
@@ -440,6 +441,9 @@ def initialize(self, api_key=None, api_secret=None, inverter_sn=None, automatic=
440441
self.automatic = automatic
441442
self.session = None
442443
self.queued_events = []
444+
# Slot events change the schedule from the caller's loop while run() copies and restores it on this one
445+
self.schedule_lock = threading.Lock()
446+
self.schedules_loaded = set() # Inverters whose schedule has been decoded, so slot events can apply to it
443447
# Last-resort fallback, used only for an LV pack that reports no BMS charge voltage to be
444448
# classified by - matches the previous hard-coded assumption (issue #4493).
445449
# get_nominal_voltage() below is the real source of truth for the full priority order.
@@ -932,6 +936,7 @@ async def decode_time_windows_v2(self, inverter_sn):
932936
"field_length": 0, # Indicate v2 format
933937
}
934938
self.charge_discharge_time_windows[inverter_sn] = result
939+
self.schedules_loaded.add(inverter_sn)
935940
self.log("Solis API: Decoded time windows v2 for {}: {}".format(inverter_sn, result)) # Debug log
936941
return result
937942

@@ -1041,21 +1046,32 @@ def restore_refused_slot_write(self, inverter_sn, slot, direction, kind, cid, at
10411046
10421047
Only restored while the schedule still holds the value that was attempted: slot events are
10431048
applied on the caller's loop, so a newer request can arrive during the write and must win.
1049+
The check and the update are made under schedule_lock so no event lands between them.
1050+
1051+
A time is restored as a whole start-end pair, because that is how the inverter holds it.
1052+
Predbat programs a slot as two select writes, start then end, back to back in
1053+
adjust_force_export(); with slot events applied immediately the end lands well under a
1054+
second after the start. The restore only happens once a refused cloud write returns, some
1055+
15-20s after run() copied the schedule, so by then the schedule holds the whole new window
1056+
and the check above leaves it alone. The one way the end can arrive after a restore is for
1057+
the start event to have been queued - only before the first decode - in which case the
1058+
slot shows the inverter's start with Predbat's end until Predbat's next execute rewrites both.
10441059
"""
10451060
inverter_value = str(self.cached_values.get(inverter_sn, {}).get(cid, ""))
1046-
slot_data = self.charge_discharge_time_windows.get(inverter_sn, {}).get(slot)
1047-
if slot_data is None:
1048-
return
1049-
if kind == "enable":
1050-
if inverter_value not in ("0", "1") or not slot_data.get(f"{direction}_enable"):
1051-
return
1052-
restored = {f"{direction}_enable": int(inverter_value)}
1053-
else:
1054-
inverter_times = inverter_value.split("-")
1055-
if len(inverter_times) != 2 or f"{slot_data.get(f'{direction}_start_time')}-{slot_data.get(f'{direction}_end_time')}" != attempted:
1061+
with self.schedule_lock:
1062+
slot_data = self.charge_discharge_time_windows.get(inverter_sn, {}).get(slot)
1063+
if slot_data is None:
10561064
return
1057-
restored = {f"{direction}_start_time": inverter_times[0], f"{direction}_end_time": inverter_times[1]}
1058-
slot_data.update(restored)
1065+
if kind == "enable":
1066+
if inverter_value not in ("0", "1") or not slot_data.get(f"{direction}_enable"):
1067+
return
1068+
restored = {f"{direction}_enable": int(inverter_value)}
1069+
else:
1070+
inverter_times = inverter_value.split("-")
1071+
if len(inverter_times) != 2 or f"{slot_data.get(f'{direction}_start_time')}-{slot_data.get(f'{direction}_end_time')}" != attempted:
1072+
return
1073+
restored = {f"{direction}_start_time": inverter_times[0], f"{direction}_end_time": inverter_times[1]}
1074+
slot_data.update(restored)
10591075
self.log(f"Warn: Solis API: {direction} slot {slot} {kind} was not accepted by {inverter_sn}, showing the inverter's {inverter_value} so Predbat writes it again")
10601076

10611077
async def write_time_windows_if_changed(self, inverter_sn):
@@ -1074,7 +1090,8 @@ async def write_time_windows_if_changed(self, inverter_sn):
10741090
return True
10751091

10761092
#  Make a copy as we will locally modify the data before writing it
1077-
time_windows = copy.deepcopy(self.charge_discharge_time_windows[inverter_sn])
1093+
with self.schedule_lock:
1094+
time_windows = copy.deepcopy(self.charge_discharge_time_windows[inverter_sn])
10781095

10791096
if self.is_tou_v2_mode(inverter_sn):
10801097
# V2 mode: check and write individual registers for changed values only
@@ -1497,6 +1514,7 @@ async def decode_time_windows(self, inverter_sn):
14971514

14981515
self.log(f"Solis API: Decoded time windows for {inverter_sn}: {result}")
14991516
self.charge_discharge_time_windows[inverter_sn] = result
1517+
self.schedules_loaded.add(inverter_sn)
15001518
return result
15011519

15021520
async def read_and_write_cid(self, inverter_sn, cid, value, field_description=None):
@@ -1529,19 +1547,21 @@ async def read_and_write_cid(self, inverter_sn, cid, value, field_description=No
15291547
self.log(f"Solis API: CID {cid} {field_description} on {inverter_sn} is set to {value}")
15301548
return True
15311549

1532-
if not result:
1533-
self.log(f"Warn: Solis API: Failed to write CID {cid} {field_description} on {inverter_sn}")
1534-
return False
1535-
15361550
# A read taken this soon after the write can still be reporting the value from before
15371551
# it, so read once more after a pause rather than concluding the write was refused
1538-
# (issue #4774). The settled read also leaves the cache holding the better answer.
1552+
# (issue #4774). The settled read also leaves the cache holding the better answer. It
1553+
# applies when the reply errored too: the value can land a moment after a reply that
1554+
# timed out, and a failure here rolls the slot schedule back to the old value.
15391555
await asyncio.sleep(self.verify_settle_seconds)
15401556
settled_value, info = await self.read_cid(inverter_sn, cid)
15411557
if cid_value_matches(settled_value, value):
15421558
self.log(f"Solis API: CID {cid} {field_description} on {inverter_sn} is set to {value}, read back {old_value} until it settled {self.verify_settle_seconds}s later")
15431559
return True
15441560

1561+
if not result:
1562+
self.log(f"Warn: Solis API: Failed to write CID {cid} {field_description} on {inverter_sn}")
1563+
return False
1564+
15451565
self.log(f"Warn: Solis API: Failed to verify CID {cid} {field_description} on {inverter_sn}, wrote {value} but read back {old_value} and still {settled_value} after {self.verify_settle_seconds}s")
15461566
return False
15471567

@@ -3244,43 +3264,49 @@ def _calculate_toggle_value(self, service, current_value):
32443264
# there is no API work to protect. They are applied here, because Predbat reads
32453265
# the entity back for about 20s after writing it and the queue is only drained
32463266
# once a minute - a queued slot time was reported as a failed write that then
3247-
# went through.
3267+
# went through. The schedule is shared with run() on the Solis loop, so every
3268+
# change to it goes through set_slot_schedule_field() under schedule_lock.
32483269
SLOT_SCHEDULE_FIELDS = {"select": ("start_time", "end_time"), "number": ("soc", "power"), "switch": ("enable",)}
32493270

32503271
def is_slot_schedule_event(self, entity_id):
3251-
"""True if entity_id is a slot schedule control of an inverter that has already been discovered."""
3272+
"""True if entity_id is a slot schedule control of an inverter whose schedule has been decoded."""
32523273
domain, _, name = entity_id.partition(".")
3253-
entity_prefix = f"{self.base.get_arg('prefix', 'predbat')}_solis_"
3274+
entity_prefix = f"{self.prefix}_solis_"
32543275
if domain not in self.SLOT_SCHEDULE_FIELDS or not name.startswith(entity_prefix):
32553276
return False
32563277
inverter_sn, _, field = name[len(entity_prefix):].partition("_")
3257-
# Until the first run() has discovered the inverter the handler would drop the event, so it waits in the queue
3258-
if self.find_inverter_by_sn(inverter_sn) is None:
3278+
# The first run() discovers the inverter before it decodes the schedule, and the decode replaces the schedule
3279+
# wholesale - an event applied in between would be lost, so until then it waits in the queue
3280+
inverter_sn = self.find_inverter_by_sn(inverter_sn)
3281+
if inverter_sn is None or inverter_sn not in self.schedules_loaded:
32593282
return False
32603283
direction, _, rest = field.partition("_slot")
32613284
slot, _, suffix = rest.partition("_")
32623285
return direction in ("charge", "discharge") and slot.isdigit() and suffix in self.SLOT_SCHEDULE_FIELDS[domain]
32633286

3264-
async def select_event(self, entity_id, value):
3265-
"""Apply a slot time now, queue anything else for the Solis loop."""
3287+
async def route_event(self, handler, entity_id, value):
3288+
"""Apply a slot schedule event now, queue anything else for the Solis loop."""
32663289
if self.is_slot_schedule_event(entity_id):
3267-
await self.select_event_handler(entity_id, value)
3290+
await handler(entity_id, value)
32683291
else:
3269-
self.queued_events.append((self.select_event_handler, entity_id, value))
3292+
self.queued_events.append((handler, entity_id, value))
3293+
3294+
async def select_event(self, entity_id, value):
3295+
"""Handle a select change from Home Assistant."""
3296+
await self.route_event(self.select_event_handler, entity_id, value)
32703297

32713298
async def number_event(self, entity_id, value):
3272-
"""Apply a slot SoC or power now, queue anything else for the Solis loop."""
3273-
if self.is_slot_schedule_event(entity_id):
3274-
await self.number_event_handler(entity_id, value)
3275-
else:
3276-
self.queued_events.append((self.number_event_handler, entity_id, value))
3299+
"""Handle a number change from Home Assistant."""
3300+
await self.route_event(self.number_event_handler, entity_id, value)
32773301

32783302
async def switch_event(self, entity_id, service):
3279-
"""Apply a slot enable now, queue anything else for the Solis loop."""
3280-
if self.is_slot_schedule_event(entity_id):
3281-
await self.switch_event_handler(entity_id, service)
3282-
else:
3283-
self.queued_events.append((self.switch_event_handler, entity_id, service))
3303+
"""Handle a switch service call from Home Assistant."""
3304+
await self.route_event(self.switch_event_handler, entity_id, service)
3305+
3306+
def set_slot_schedule_field(self, inverter_sn, slot_num, field, value):
3307+
"""Set one field of a slot in the in-memory schedule, under the lock run() copies and restores it under."""
3308+
with self.schedule_lock:
3309+
self.charge_discharge_time_windows.setdefault(inverter_sn, {}).setdefault(slot_num, {})[field] = value
32843310

32853311
async def select_event_handler(self, entity_id, value):
32863312
"""Handle select entity changes"""
@@ -3334,15 +3360,10 @@ async def select_event_handler(self, entity_id, value):
33343360
new_hhmm = value[:5] if len(value) >= 5 else value
33353361

33363362
# Update charge_discharge_time_windows cache
3337-
if inverter_sn not in self.charge_discharge_time_windows:
3338-
self.charge_discharge_time_windows[inverter_sn] = {}
3339-
if slot_num not in self.charge_discharge_time_windows[inverter_sn]:
3340-
self.charge_discharge_time_windows[inverter_sn][slot_num] = {}
3341-
33423363
if "start_time" in field:
3343-
self.charge_discharge_time_windows[inverter_sn][slot_num]["charge_start_time"] = new_hhmm
3364+
self.set_slot_schedule_field(inverter_sn, slot_num, "charge_start_time", new_hhmm)
33443365
elif "end_time" in field:
3345-
self.charge_discharge_time_windows[inverter_sn][slot_num]["charge_end_time"] = new_hhmm
3366+
self.set_slot_schedule_field(inverter_sn, slot_num, "charge_end_time", new_hhmm)
33463367

33473368
# Write will happen in the main loop
33483369

@@ -3366,15 +3387,10 @@ async def select_event_handler(self, entity_id, value):
33663387
new_hhmm = value[:5] if len(value) >= 5 else value
33673388

33683389
# Update charge_discharge_time_windows cache
3369-
if inverter_sn not in self.charge_discharge_time_windows:
3370-
self.charge_discharge_time_windows[inverter_sn] = {}
3371-
if slot_num not in self.charge_discharge_time_windows[inverter_sn]:
3372-
self.charge_discharge_time_windows[inverter_sn][slot_num] = {}
3373-
33743390
if "start_time" in field:
3375-
self.charge_discharge_time_windows[inverter_sn][slot_num]["discharge_start_time"] = new_hhmm
3391+
self.set_slot_schedule_field(inverter_sn, slot_num, "discharge_start_time", new_hhmm)
33763392
elif "end_time" in field:
3377-
self.charge_discharge_time_windows[inverter_sn][slot_num]["discharge_end_time"] = new_hhmm
3393+
self.set_slot_schedule_field(inverter_sn, slot_num, "discharge_end_time", new_hhmm)
33783394

33793395
# Write will happen in the main loop
33803396

@@ -3428,12 +3444,7 @@ async def number_event_handler(self, entity_id, value):
34283444
return
34293445

34303446
# Update charge_discharge_time_windows cache
3431-
if inverter_sn not in self.charge_discharge_time_windows:
3432-
self.charge_discharge_time_windows[inverter_sn] = {}
3433-
if slot_num not in self.charge_discharge_time_windows[inverter_sn]:
3434-
self.charge_discharge_time_windows[inverter_sn][slot_num] = {}
3435-
3436-
self.charge_discharge_time_windows[inverter_sn][slot_num]["charge_soc"] = float(value_str)
3447+
self.set_slot_schedule_field(inverter_sn, slot_num, "charge_soc", float(value_str))
34373448

34383449
# Write will happen in the main loop
34393450

@@ -3457,12 +3468,7 @@ async def number_event_handler(self, entity_id, value):
34573468
amps = int(value / self.get_nominal_voltage(inverter_sn))
34583469

34593470
# Update charge_discharge_time_windows cache
3460-
if inverter_sn not in self.charge_discharge_time_windows:
3461-
self.charge_discharge_time_windows[inverter_sn] = {}
3462-
if slot_num not in self.charge_discharge_time_windows[inverter_sn]:
3463-
self.charge_discharge_time_windows[inverter_sn][slot_num] = {}
3464-
3465-
self.charge_discharge_time_windows[inverter_sn][slot_num]["charge_current"] = float(amps)
3471+
self.set_slot_schedule_field(inverter_sn, slot_num, "charge_current", float(amps))
34663472

34673473
# Write will happen in the main loop
34683474

@@ -3483,12 +3489,7 @@ async def number_event_handler(self, entity_id, value):
34833489
return
34843490

34853491
# Update charge_discharge_time_windows cache
3486-
if inverter_sn not in self.charge_discharge_time_windows:
3487-
self.charge_discharge_time_windows[inverter_sn] = {}
3488-
if slot_num not in self.charge_discharge_time_windows[inverter_sn]:
3489-
self.charge_discharge_time_windows[inverter_sn][slot_num] = {}
3490-
3491-
self.charge_discharge_time_windows[inverter_sn][slot_num]["discharge_soc"] = float(value_str)
3492+
self.set_slot_schedule_field(inverter_sn, slot_num, "discharge_soc", float(value_str))
34923493

34933494
# Write will happen in the main loop
34943495

@@ -3512,12 +3513,7 @@ async def number_event_handler(self, entity_id, value):
35123513
amps = round(float(value) / self.get_nominal_voltage(inverter_sn), 1)
35133514

35143515
# Update charge_discharge_time_windows cache
3515-
if inverter_sn not in self.charge_discharge_time_windows:
3516-
self.charge_discharge_time_windows[inverter_sn] = {}
3517-
if slot_num not in self.charge_discharge_time_windows[inverter_sn]:
3518-
self.charge_discharge_time_windows[inverter_sn][slot_num] = {}
3519-
3520-
self.charge_discharge_time_windows[inverter_sn][slot_num]["discharge_current"] = amps
3516+
self.set_slot_schedule_field(inverter_sn, slot_num, "discharge_current", amps)
35213517

35223518
# Write will happen in the main loop
35233519

@@ -3641,12 +3637,7 @@ async def switch_event_handler(self, entity_id, service):
36413637
return
36423638

36433639
# Update charge_discharge_time_windows cache
3644-
if inverter_sn not in self.charge_discharge_time_windows:
3645-
self.charge_discharge_time_windows[inverter_sn] = {}
3646-
if slot_num not in self.charge_discharge_time_windows[inverter_sn]:
3647-
self.charge_discharge_time_windows[inverter_sn][slot_num] = {}
3648-
3649-
self.charge_discharge_time_windows[inverter_sn][slot_num]["charge_enable"] = int(value)
3640+
self.set_slot_schedule_field(inverter_sn, slot_num, "charge_enable", int(value))
36503641

36513642
# Write will happen in the main loop
36523643

@@ -3678,12 +3669,7 @@ async def switch_event_handler(self, entity_id, service):
36783669
return
36793670

36803671
# Update charge_discharge_time_windows cache
3681-
if inverter_sn not in self.charge_discharge_time_windows:
3682-
self.charge_discharge_time_windows[inverter_sn] = {}
3683-
if slot_num not in self.charge_discharge_time_windows[inverter_sn]:
3684-
self.charge_discharge_time_windows[inverter_sn][slot_num] = {}
3685-
3686-
self.charge_discharge_time_windows[inverter_sn][slot_num]["discharge_enable"] = int(value)
3672+
self.set_slot_schedule_field(inverter_sn, slot_num, "discharge_enable", int(value))
36873673

36883674
# Write will happen in the main loop not here
36893675

0 commit comments

Comments
 (0)