Repository navigation
Conversation
…ng sets rate to 0
…ual_api override commands
springfall2008
left a comment
There was a problem hiding this comment.
Automated comment from the triage bot.
Reviewed at 1740275. The rewrite is sound overall: I traced the old write conditions (the 7 block claims + the final if resetPause: reset on main) against the new single merged write in apply_inverter_rates() and they cover the same cases, with the limit-of-0 and balancer-hold sources added as intended; balance_inverters() mutates only rate keys so plan pause flags survive on the balancer path; and the release paths (limit raised, hold lifted, applied on plan runs and on the 60s poll) all land on the same write. 2 findings, both low: 1 inline (docs), 1 in the body (a pause-write gap during calibration cycles).
apps/predbat/execute.py:542-543 (body) — pause writes stop for already-processed inverters on a cycle where a later inverter is calibrating. The calibration branch does intent.clear() + break, so apply_rate_intent({}) runs and nothing — including pause mode — is written for inverters processed before the calibrating one. On main the pause writes happened inside those blocks, so they were made even on such a cycle. Scenario: a two-inverter fleet where inverter 1 (later in self.inverters) reports battery_calibration active for a few hours; set_freeze_export_during_demand is on, or a limit of 0 is set. Every execute cycle during those hours, inverter 0's pause is neither set nor released — on main it was written every cycle — so cross-charge prevention silently stops applying to it (and if it was already paused, it stays latched). Not covered by the PR's tests and not in the PR body's disclosed differences; the exception is also missing from the new comment at apps/predbat/execute.py:456. Note the calibrating fleet is a deliberate stand-down and the calibration branch itself resets rates to max for the whole fleet (execute.py:528-532), so this may be acceptable — flagging because the comment and the PR body claim the single write covers everything. (Verified by reading execute_plan()'s calibration branch at 528-543 and apply_rate_intent()'s skip-of-missing-ids at 264-266; no test at that head exercises a calibration cycle together with a pause-claiming block.)
springfall2008
left a comment
There was a problem hiding this comment.
Automated comment from the triage bot.
Re-reviewed at 938e473 ("respond to bot review comments") — both findings from the review at 17402756 are addressed. I verified each fix against the code at this head. 0 new findings (0 inline, 0 in body).
-
docs/what-does-predbat-do.md — mode bullets. The Monitor and Control SoC only bullets now each name both exceptions, and both exceptions are real in the code: the Freeze Export during Demand block is not gated on the charge/export windows or the mode select (execute.py:950-964), so it claims a charge pause with both windows off; and the balancer writes through
apply_rate_intent()→apply_inverter_rates(), whose pause write fires whenever a pause is requested regardless of mode (execute.py:344-345). For a direction the executor never claimed (None),balance_inverters()treats the inverter's own maximum as the rate in force, so it can take an unclaimed direction to 0 in Control SoC only and Monitor too (utils.py:2905-2915); the poll gates only on the balance switch and read-only (execute.py:1611). -
Calibration stand-down (apps/predbat/execute.py:463 comment, plus the new
test_calibration_writes_no_pause). The exception is now disclosed at the sentence that claims the single write covers everything, and the new test pins the behaviour. Its premises check out against the code at this head: a calibration cycle breaks out with a cleared intent, soapply_rate_intent()writes nothing — pause mode included — for the inverters processed before the calibrating one (execute.py:527-545, skip at 264-266, call at 1205); the pause flags the test reads are the test double'sadjust_pause_modeoutputs (test_execute.py:153-155), so the assertions are real rather than vacuous; the 60s poll declines a calibrating fleet and drops the stored intent (execute.py:1618-1621) and the plan-run balancer stops too (utils.py:2886-2890), so no stale plan's pause is applied; and the next normal cycle writes the correct pause again (execute.py:340-345).
No new issues in the +95/−4 delta.
Set battery pause mode when a charge/discharge limit or balancing holds a rate at 0
Fixes #2155
Problem
Setting
inverter_limit_chargeorinverter_limit_dischargeto 0 (for example viaselect.predbat_manual_api) stops Predbat asking for charge or discharge, but many inverters still trickle at 150-200W when the rate is 0.Inverter balancing has the same problem: it takes a rate to 0 but the battery keeps trickling.
On inverters that have a timed pause mode (
has_timed_pause, e.g. GivEnergy) only the pause mode stops this.What this does
If the inverter has a pause mode, Predbat now sets it to match:
The same happens when balancing holds an inverter's charge or discharge rate at 0, and the pause is released when the limit is raised or the hold ends. Inverters without a pause mode are unchanged, because
Inverter.adjust_pause_mode()returns immediately for them.It only applies when Predbat is controlling the charge/export windows (Control charge, Control charge & discharge). Monitor and Control SoC only never write a pause for a limit of 0, consistent with them not writing rates.
Changes to
execute.pyThe pause mode used to be written from inside the
execute_plan()loop, from several separate blocks (the charge hold, Freeze Export, the car hold, the iBoost hold, and a final reset), while rates were written afterwards byapply_rate_intent()/apply_inverter_rates(). A limit of 0 and the balancer both act after the loop, so a second pause write in the loop could not see them, and a second write elsewhere would just undo the first. So the pause write now happens in one place:execute_plan(): the 7 directadjust_pause_mode(pause_charge/discharge=True)calls and the final reset call are removed. The blocks still setpause_charge_requested/pause_discharge_requestedandresetPauseexactly as before, and those flags are already carried in the per-inverterintent(pause_charge/pause_discharge). Nothing about when a block asks for a pause has changed. I also removed an unusedmax_ratevariable and added a comment at the top of the function and at each removed call explaining where the write went.apply_inverter_rates(): after the existing rate writes, it makes oneadjust_pause_mode()call whose arguments are the OR of (a) the plan's own request from the intent, (b) a limit of 0, i.e.battery_rate_max_charge/_discharge <= 0(these are the hardware maximum reduced byinverter_limit_charge/_discharge, so 0 only means the user set a limit of 0), and (c) a balancer hold. The call is made whenreset_ratesis true (the same condition as the old final reset, i.e. Predbat controls the windows) or when any pause is requested. Two new keyword arguments,balance_pause_charge/balance_pause_discharge, default toFalse, so existing callers are unaffected.apply_rate_intent(): works out the two balancer flags, which are true when the balancer has overridden a rate (it differs from the plan's own rate) and taken it to 0 or below, and passes them on. It runs on the once-a-minute balancing poll as well as at plan time, so the pause tracks the balancer hold as it starts and ends.Why this is safe for the rest of
execute.pyresetPausegating, are untouched. The old code calledadjust_pause_mode()in exactly the cases where a block claimed a pause or the final reset ran; the new write condition (reset_rates or a pause requested) covers the same cases, with the flags carried in the intent.reset_rates, so with both windows off nothing new is written. The plan's own Freeze Export during Demand pause, which is not gated on the windows, still works as before.adjust_pause_mode()returns early withoutinv_has_timed_pause, so nothing happens there.adjust_pause_mode()only writes when the value differs.Three small behaviour differences to be aware of:
execute_plan()call.Known edge: if an inverter's configured maximum rate were ever 0 through a sensor error, it would now also be paused. The default maximum is 2600W, so this needs a misread sensor to reach.
New tests (
test_execute.py:test_rate_limit_zero_pauses_battery)All run on the existing mock inverters in the
executetest group:apply_inverter_rates(): neither limit, charge limit 0, discharge limit 0, both 0 give no pause, Pause charge, Pause discharge and Pause both. Also a plan-requested pause merged with a limit of 0, and a plan-only pause.reset_ratesfalse) with limits of 0 writes no pause, with an error message explaining that monitor mode must not change the inverter.apply_rate_intent(): a charge rate taken to 0 on one inverter pauses only that inverter's charge, and re-applying the plan without the override releases it.execute_plan()holds the cross-charging inverter at 0 and pauses its charge only.execute_plan()with both limits at 0 pauses both inverters.Docs
manual-api.md: new section oninverter_limit_charge/_dischargeof 0 with the table above. Also added- inverter_limit_charge_dcto the list of supported overrides, missing from the docs and highlighted by the bot on feat(api): let the manual API override inverter_limit_export #5839what-does-predbat-do.md: added explainer of what Predbat writes in each mode (Monitor, Control SoC only, Control charge, Control charge & discharge).customisation.md: balance inverters now sets the pause mode alongside the rates where the inverter supports it.Testing
./run_allpasses; pre-commit passes.