Skip to content

Commit 47d3c48

Browse files
johanzanderclaude
andauthored
fix: release VPP control when IDLE at the reserve floor (#592) (#619)
* fix: release VPP control when IDLE at the reserve floor (#592) Growatt VPP mapped every IDLE period to vpp_power=+1 with remote control enabled ("battery first"), regardless of state of charge. Because remote control stays enabled, _apply_period_vpp rewrites the command every period to refresh the inverter's fallback timer, so through a long overnight idle the inverter is never handed back and its BMS never idles down. Reported on real hardware. The battery_first hold exists to protect stored energy from self-consumption (#466): IDLE's own DP cost model never credits battery discharge for load, so self-consumption must come from grid/solar. At the configured minimum SoE there is no stored energy left to protect, so the hold buys nothing and costs the inverter its sleep. IDLE at the floor now returns (0, False) -- released to the inverter's own load_first self-use. Above the floor nothing changes, so #466 is preserved. Releasing rather than writing power=0 with remote control still enabled (grid_first) is what keeps this flow-neutral. load_first still absorbs passive solar surplus exactly as the battery_first hold does, where grid_first holds against charging and would bypass that surplus to the grid -- a real change, since IDLE's DP cost model does credit that absorption. The v10.0.2 VPP regression baseline is unchanged on all 37 fixtures, 24 of which contain IDLE-at-floor periods. at_reserve_floor is derived live in BatterySystemManager._at_reserve_floor() and threaded through apply_period the same way block_passive_charging (#355) and strategic_intent (#413) already are. It is read fresh at every write -- including the retry path and the discharge-inhibit path, both of which would otherwise default it to False and silently re-assert the hold. Caveat, documented in INVERTER_PLATFORMS.md: how far the battery can fall under released self-use is the inverter's own discharge_stop_soc, and VPP mode never writes that register (#309). Not new here -- LOAD_SUPPORT and SOLAR_STORAGE already release control at any SoC -- but it means the fix ships experimental pending the reporter's hardware confirmation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tmoey3FXSMsMsmVv8zmKNB * fix: wire at_reserve_floor through display and simulator, guard unreadable SoC Addresses both Stage 4 reviews on #619. - _at_reserve_floor() reuses _get_current_battery_soc()'s validation and holds (does not release) on an unreadable sensor, logged explicitly. Previously float * None raised an uncaught TypeError from two unguarded apscheduler jobs, on every platform. - _planned_at_reserve_floor() derives the flag from the plan's SoE trajectory and feeds _vpp_display_state, so the schedule API no longer reports the old hold for periods production releases. - derive_vpp_commands threads a per-period at_reserve_floor from the simulated SoE trajectory, so the VPP fixture corpus actually exercises the new branch. Refs #592 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QiPyLE5Kz9RvNiQemAdjym * test: re-pin the VPP baseline commands, plans untouched Both halves of the pin moved once derive_vpp_commands actually receives at_reserve_floor -- including test_vpp_execution_of_the_baseline_plan_is_unchanged, since this PR edits the execution model that half replays the historical plan through. --repin-current skips that half by design, so the historical plans were extracted from the baseline's own plan fields and replayed via --from-plans, then --add-new restored the three post-tag plan:null fixtures. No tag checkout, so the v10.0.2 plans are byte-identical. half entries periods d(cost) d(SoE) v10.0.2 (hist) 23 234 0.000000000000 0.000000000000 current 27 265 0.000000000000 0.000000000000 499 periods change [1, True] -> [0, False]; nothing moves a joule or an ore. Verified after writing: fixture set identical, zero plans altered on either half, only the 50 command arrays re-pinned -- so the drift signal that test_drift_from_the_released_version_is_recorded depends on survives intact. Refs #592 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QiPyLE5Kz9RvNiQemAdjym * fix: read the entering SoE, and drop the getattr masking it Both findings from the Stage 4 round on 43295e6, confirmed against source. 1. rules.md:57 forbids getattr(obj, key, default). state_of_energy is a required constructor arg on DPSchedule, so the attribute always exists on a real instance -- the getattr was masking current_schedule being None, which is now checked directly. 2. Off-by-one. state_of_energy is combined_soe, whose only writer stores period_data.energy.battery_soe_end (battery_system_manager.py:2254); battery_soe_start and battery_soe_end are distinct fields on EnergyData. So index p is the SoE LEAVING period p, and the SoE entering it is p-1. Reading p reported the release one period early at the crossing, and one late on the way back up -- the exact display/write disagreement the method exists to prevent. The existing display tests could not see this: their fixtures pin state_of_energy to a uniform value, where p and p-1 agree by construction. test_the_crossing_period_still_displays_the_hold varies the trajectory across the boundary and fails on the old index. Removing the getattr also surfaced a fixture that was never shaped like a real DPSchedule: MagicMock(spec=DPSchedule) specs off the class, so it omits state_of_energy, which __init__ sets. The silent fallback had been hiding it. Refs #592 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QiPyLE5Kz9RvNiQemAdjym * docs: say what the VPP baseline actually shows Two doc passages still asserted the claim this PR withdrew -- that the v10.0.2 VPP baseline is unchanged. It is not, and it could not have been: before at_reserve_floor reached derive_vpp_commands the branch was unreachable from the capture harness, so an unchanged baseline meant the change was never exercised, not that it was flow-neutral. Both now state what is actually unchanged. Commands move at every IDLE-at-floor period (499 across 50 entries, [1, true] -> [0, false]); realized cost and the SoE trajectory are bit-identical, 0.000000000000 on both halves. Commands moving with the energy fixed is the evidence. This matters most in bess-knowledge.md, which CLAUDE.md designates as ground truth for any agent answering BESS behaviour questions -- a wrong claim there misinforms future work rather than just this PR. Also corrects _planned_at_reserve_floor's docstring, which justified reading index 0 for period 0 by asserting period 0 is always the optimization period. It is not: on a schedule re-optimized mid-day, index 0 is historical and holds battery_soe_end like every other index, so the read is one period early there. Display-only and bounded to period 0. Stated rather than papered over -- the array has no entering value for period 0 to read, so recording one is a change to _create_updated_schedule, not to this method. Refs #592 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QiPyLE5Kz9RvNiQemAdjym --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent beaf562 commit 47d3c48

16 files changed

Lines changed: 1912 additions & 1180 deletions

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
2222

2323
### Fixed
2424

25+
- **Growatt VPP now lets the inverter and BMS sleep through a long idle at minimum SoC** — an empty battery was still held under continuous remote control, which nothing was protecting. ([#592](https://github.com/johanzander/bess-manager/issues/592))
2526
- **A tiny solar surplus is no longer planned as an export the inverter will absorb** — below the export the plan can express, the battery charged anyway and ran fuller than planned, spilling the difference later. ([#630](https://github.com/johanzander/bess-manager/issues/630))
2627
- **The setup wizard no longer locks you out of an inverter platform it failed to auto-detect** — every platform stays selectable, and a re-scan keeps the one you picked. ([#621](https://github.com/johanzander/bess-manager/issues/621))
2728
- **System no longer gets stuck on "initializing" when many consecutive periods are near-tied** — a long run of volatile prices could make every hourly optimization fail, leaving no schedule at all. ([#624](https://github.com/johanzander/bess-manager/issues/624))

core/bess/battery_system_manager.py

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2715,6 +2715,8 @@ def _apply_period_schedule(self, period: int) -> None:
27152715
error=e,
27162716
)
27172717

2718+
at_reserve_floor = self._at_reserve_floor()
2719+
27182720
# Store the schedule's desired discharge rate before inhibit check so that
27192721
# apply_discharge_inhibit() can restore it when the inhibit sensor clears.
27202722
self._desired_discharge_rate = discharge_rate
@@ -2764,6 +2766,7 @@ def _apply_period_schedule(self, period: int) -> None:
27642766
discharge_rate,
27652767
block_passive_charging,
27662768
strategic_intent,
2769+
at_reserve_floor,
27672770
)
27682771

27692772
if not success:
@@ -2790,6 +2793,51 @@ def _apply_period_schedule(self, period: int) -> None:
27902793
# Apply charging power rate (BSM-level concern: uses power monitor)
27912794
self.adjust_charging_power()
27922795

2796+
def _at_reserve_floor(self) -> bool:
2797+
"""Whether the battery is sitting on its reserve floor right now (#592).
2798+
2799+
Read live rather than taken from the plan: an IDLE hold exists to
2800+
protect stored energy from self-consumption, so what decides whether
2801+
the hold is worth anything is whether energy is actually there now. A
2802+
plan that expected a reserve does not mean one survived.
2803+
2804+
Called fresh at each write, including retries minutes later, for the
2805+
same reason -- a captured flag would command the inverter on a SoC
2806+
that has since moved.
2807+
2808+
The SoE conversion deliberately mirrors `min_soe_kwh`'s own
2809+
(`total_capacity * pct / 100`, settings.py) rather than the equivalent
2810+
`pct / 100 * total_capacity`. The two can differ in the last bit, and
2811+
the case that decides this branch is exact equality -- a battery
2812+
parked on its floor overnight, which is precisely the reported
2813+
scenario.
2814+
2815+
**An unreadable SoC holds, and says so.** `get_battery_soc()` is
2816+
`float | None`, so a transient unavailable/unknown sensor must be
2817+
decided here rather than propagating: this runs for every platform on
2818+
every period write, and two of its callers (the retry closure's
2819+
apscheduler job and the every-minute discharge-inhibit job) have no
2820+
exception handling at all, so raising would take down far more than
2821+
this flag. Holding is chosen over releasing because it is the safe
2822+
direction and is exactly the pre-#592 behaviour -- releasing is what
2823+
could let the inverter's own self-use draw the battery down, so it
2824+
must never happen on a reading we could not verify. This is an
2825+
explicit, logged branch, not a silent fallback: rules.md forbids
2826+
degrading quietly, not choosing a safe outcome loudly.
2827+
2828+
Validation is `_get_current_battery_soc()`'s, reused rather than
2829+
restated, so the definition of a valid reading stays in one place.
2830+
"""
2831+
soc = self._get_current_battery_soc()
2832+
if soc is None:
2833+
logger.warning(
2834+
"Reserve-floor check: SoC unreadable — holding the battery "
2835+
"(not releasing VPP control) until a valid reading returns"
2836+
)
2837+
return False
2838+
current_soe = self.battery_settings.total_capacity * soc / 100.0
2839+
return current_soe <= self.battery_settings.min_soe_kwh
2840+
27932841
_PERIOD_RETRY_DELAYS_MIN: ClassVar[list[int]] = [
27942842
3,
27952843
8,
@@ -2838,6 +2886,7 @@ def retry_period_write():
28382886
discharge_rate,
28392887
block_passive_charging,
28402888
strategic_intent,
2889+
self._at_reserve_floor(),
28412890
)
28422891
self._runtime_failure_tracker.dismiss_by_category("period_apply")
28432892
if not success:
@@ -3456,6 +3505,10 @@ def apply_discharge_inhibit(self) -> None:
34563505
target_rate,
34573506
self._desired_block_passive_charging,
34583507
self._desired_strategic_intent,
3508+
# Fresh, not the value from the scheduled write: this runs
3509+
# mid-period, and omitting it would default to False and
3510+
# re-assert the battery_first hold #592 released.
3511+
self._at_reserve_floor(),
34593512
)
34603513
self._last_applied_discharge_rate = target_rate
34613514

core/bess/inverter_controller.py

Lines changed: 61 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -578,6 +578,7 @@ def apply_period(
578578
discharge_rate: int,
579579
block_passive_charging: bool = False,
580580
strategic_intent: str = "",
581+
at_reserve_floor: bool = False,
581582
) -> tuple[bool, str]:
582583
"""Write period control settings to hardware.
583584
@@ -599,6 +600,12 @@ def apply_period(
599600
BATTERY_EXPORT to the same values, so platforms that need to
600601
treat them differently (VPP-style -- see #413) require the
601602
intent itself. Register-based platforms ignore this.
603+
at_reserve_floor: Whether the battery is at (or below) its
604+
configured minimum SoE right now. Register-based platforms
605+
ignore this -- their min_soc register already stops discharge
606+
at the floor. Forced-power platforms use it to stop holding a
607+
battery that has nothing left to hold, releasing the inverter
608+
so its BMS can sleep -- see #592.
602609
603610
Returns:
604611
Tuple of (success, error_message). error_message is empty on success.
@@ -668,16 +675,64 @@ def get_period_settings(self, period: int) -> dict:
668675
"discharge_rate": discharge_rate,
669676
"strategic_intent": intent,
670677
**self._mode_display_fields(
671-
intent, grid_charge, discharge_rate, block_passive_charging
678+
intent,
679+
grid_charge,
680+
discharge_rate,
681+
block_passive_charging,
682+
self._planned_at_reserve_floor(period),
672683
),
673684
}
674685

686+
def _planned_at_reserve_floor(self, period: int) -> bool:
687+
"""Whether the *plan* has the battery on its reserve floor entering
688+
this period (#592).
689+
690+
The display counterpart to `BatterySystemManager._at_reserve_floor()`,
691+
which reads live SoC. A displayed period is a prediction, so the plan's
692+
own SoE trajectory is the correct input -- but it must answer the same
693+
question, or the UI shows a hold for periods production releases and
694+
`_mode_display_fields` breaks its own no-fabrication contract.
695+
696+
**Index p-1, not p.** `state_of_energy` is `combined_soe`, whose only
697+
writer stores `period_data.energy.battery_soe_end` (see
698+
`BatterySystemManager._create_updated_schedule`) -- `battery_soe_end`
699+
and `battery_soe_start` are distinct fields on `EnergyData`. So
700+
`state_of_energy[p]` is the SoE *leaving* period p, and the SoE
701+
*entering* it is index p-1. Reading index p directly would answer for
702+
the wrong period: at the boundary where the plan first reaches the
703+
floor it would report "at floor" one period early, and one period late
704+
on the way back up -- exactly the display/write disagreement this
705+
method exists to prevent.
706+
707+
Period 0 has no predecessor in the array, so it reads index 0. That is
708+
exact only when period 0 *is* the optimization period, where
709+
`combined_soe[0]` is written as `current_soe` — an entering value. On a
710+
schedule re-optimized mid-day, index 0 is a historical period holding
711+
`battery_soe_end` like every other index, so the read is one period
712+
early there. Display-only and bounded to period 0: `get_period_settings`
713+
never writes hardware, and the live write path reads SoC directly. Left
714+
as-is rather than papered over, because the array has no entering value
715+
for period 0 to read — recording one is a change to
716+
`_create_updated_schedule`, not to this method.
717+
718+
False when there is no plan to read: with no trajectory there is no
719+
prediction to display, and the hold is the unchanged-behaviour answer.
720+
"""
721+
if self.current_schedule is None:
722+
return False
723+
soe = self.current_schedule.state_of_energy
724+
if period >= len(soe):
725+
return False
726+
entering_soe = soe[period - 1] if period > 0 else soe[0]
727+
return entering_soe <= self.battery_settings.min_soe_kwh
728+
675729
def _mode_display_fields(
676730
self,
677731
intent: str,
678732
grid_charge: bool,
679733
discharge_rate: int,
680734
block_passive_charging: bool,
735+
at_reserve_floor: bool = False,
681736
) -> dict:
682737
"""Single source of truth for what mode-related fields a period
683738
gets, branching on CONTROL_MODEL. Never fabricates a label the
@@ -704,7 +759,11 @@ def _mode_display_fields(
704759
# SolaxModbusGrowattController) -- no hasattr() duck-typing on a
705760
# subclass-private method name.
706761
power_pct, remote_control = self._vpp_display_state(
707-
grid_charge, discharge_rate, block_passive_charging, intent
762+
grid_charge,
763+
discharge_rate,
764+
block_passive_charging,
765+
intent,
766+
at_reserve_floor,
708767
)
709768
return {
710769
"vpp_power_pct": power_pct,

0 commit comments

Comments
 (0)