feat(octopus): gate IOG dispatch-slot trust on a real car_charging_now sensor (#4482, #4516) - #5110
chalfontchubby wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Two critical and three moderate findings remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds opt-in safeguards for trusting future Octopus Intelligent dispatch slots, including charging-sensor confirmation and configurable need/trust gates.
Changes:
- Adds configurable dynamic-slot trust and EV-need gates.
- Tracks per-car
car_charging_nowconfirmation streaks. - Updates rate handling, configuration, documentation, runtime state, and tests.
File summaries
| File | Reviewed changes and findings |
|---|---|
docs/car-charging.md |
Documents the new controls. Nit (3 votes): clarify that none keeps completed dispatches at the normal rate. |
apps/predbat/unit_test.py |
Registers the new test suite. No final comments. |
apps/predbat/tests/test_rate_add_io_slots.py |
Adds rate-gating tests. Nit (1 vote): add coverage for the feed-side exclusion path. |
apps/predbat/tests/test_fetch_config_options.py |
Adds configuration tests. No final comments. |
apps/predbat/tests/test_car_charging_now_confirmed_slots.py |
Tests charging confirmation streaks. Moderate (1 vote): isolate or fully restore shared fixture state. |
apps/predbat/predbat.py |
Initializes new runtime state. No final comments. |
apps/predbat/octopus.py |
Implements trust-gated rate handling. Critical (1 vote): untrusted overlaps can block later trusted slots. Moderate (1 vote): cap-rejected slots can lose valid feed discounts. Nit (1 vote): feed-side exclusion lacks regression coverage. |
apps/predbat/fetch.py |
Adds sensor detection and configuration handling. Critical (1 vote): confirmation changes may not update the change signature or force replanning. Moderate (1 vote): zero-car configurations can record a spurious error. |
apps/predbat/const.py |
Defines confirmation timing constants. No final comments. |
apps/predbat/config.py |
Adds expert configuration options. No final comments. |
Review details
Suppressed comments (5)
apps/predbat/fetch.py:2539
- The raw-config check treats any scalar value as a configured sensor, including the valid fixed
sensor_type: string|booleanvalues such ascar_charging_now: "yes". Withstarted, that constant produces trusted streaks without any charger/car sensor and can make provisional slots cheap; require a non-empty entity-id string (and the same check for list entries) before returningTrue.
configured = self.args.get("car_charging_now", None)
if configured is None:
return False
if not isinstance(configured, list):
# A scalar (un-indexed) value applies to every car, not just car 0: resolve_arg() only
# indexes when the configured value is itself a list (userinterface.py - "isinstance(
# value, list) and index is not None"), so a single sensor resolves identically for
# every car_n. Returning car_n == 0 here would wrongly degrade car 1+ to "completed" on
# a single-sensor multi-car install whose sensor does in fact cover them.
return True
return car_n < len(configured) and configured[car_n] not in (None, "")
apps/predbat/fetch.py:3262
num_carsexplicitly supports zero (config.py:2684), but this loop forces a fictitious car 0 and recordshad_errors=Truewhenever a car-less install selectsstarted. Sincerate_add_io_slots()never processes any car whennum_cars == 0, this creates a spurious error on an otherwise valid configuration; iterate only overrange(self.num_cars).
for car_n in range(max(self.num_cars, 1)):
if not self.has_car_charging_now_sensor(car_n):
apps/predbat/octopus.py:3574
- This strips the feed-side discount from a future slot rejected only because
octopus_slot_maxwas already reached.rate_add_io_slots()intentionally does not add cap-rejected minutes totrusted_dynamic_minuteswhenneededandtrustedare both true (lines 3476-3517), so this loop cannot distinguish that case from an untrusted/withdrawn slot and restoresrate_max_baseanyway. That contradicts the documented cap-only exemption and makes the stricter trust modes overrule a genuine dispatch solely because Predbat's local budget was full; preserve cap-only minutes separately (or otherwise carry that reason through before stripping feed adjustments).
for minute in list(self.io_adjusted.keys()):
if minute < self.minutes_now:
continue
if self.io_adjusted[minute] and not self.minute_in_iog_fixed_window(minute) and minute not in self.trusted_dynamic_minutes:
rates[minute] = self.rate_max_base
apps/predbat/octopus.py:3542
- The new feed-side exclusion path is not exercised by the added tests:
test_rate_add_io_slots.pycallsrate_add_io_slots()directly and manually setsio_adjusted, but no test callsexclude_dynamic_io_slots()or the full fetch pipeline. This leaves the safety-critical case where a futureio_adjustedminute is restored and its marker cleared, while past, fixed-window, and trusted minutes remain untouched, unverified. Add a focused regression test.
def exclude_dynamic_io_slots(self, rates):
apps/predbat/tests/test_car_charging_now_confirmed_slots.py:317
- This test restores only a few fields, but each
get_car_charging_planned()call overwrites the shared fixture'scar_charging_planned, plan arrays, battery size/limit/rate, charging slots, exclusivity, and related state. The final call rebuilds those values fromold_argsrather than restoring values set by the preceding test;unit_test.pyreuses one PredBat instance for the registry, so later tests can inherit this mutation. Snapshot and restore all mutated fields or use an isolated fixture.
my_predbat.get_car_charging_planned()
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # since get_state_wrapper() may return a cached or shared list. | ||
| if completed: | ||
| self.octopus_slots[car_n] += completed | ||
| self.octopus_slots[car_n] += [dict(slot, _confirmed=True) for slot in completed] |
There was a problem hiding this comment.
(Written by Claude, working with @chalfontchubby.)
Good catch - fixed. _confirmed is now part of the signature, in both branches.
The in-progress branch mattered most: that keeps only the stable fields precisely because start/charge_in_kwh re-clock every refresh, and a slot is most likely to be confirmed while it is actually running - so without it there, a mid-slot confirmation would be the case most likely to be missed.
Covered by two new tests in test_octopus_slots_change.py (5b and 5c), both verified to fail without the fix.
| # At the start of each 30-min slot, decide if we can add it | ||
| if minute % 30 == 0: | ||
| if slots_per_day[day_offset] < octopus_slot_max: | ||
| if needed and trusted and slots_per_day[day_offset] < octopus_slot_max: |
There was a problem hiding this comment.
(Written by Claude, working with @chalfontchubby.)
I tried to reproduce this one and could not, so I have left the code alone rather than change behaviour on a finding I cannot demonstrate.
Setup: started, a sensor-corroborated current block (10:00-10:30), an unconfirmed planned slot covering it, and add_now_to_octopus_slot()s live slot for the same block appended after it - the exact ordering described. Both with and without a fix, minutes 600-629 came out at rate_min_base. The discount applied either way.
I also wrote a test for it first, which passed against the unfixed code - so it was vacuous. The reason is worth recording: rate_max_base is 0 on the bare test fixture, the same as an unset rate_min_base, so an assertion comparing the two passes whichever branch ran. Giving them distinct values still showed no difference between fixed and unfixed.
That does not prove the concern is wrong - saved_slots claiming a minute before the needed/trusted gate is still questionable in principle, and the dedup only bites on partial overlaps, which I have not explored exhaustively. But shipping an untestable change would be worse than leaving it. If you have a concrete slot arrangement that shows the discount being lost, I will happily take it - a failing case is all this needs.
| A daytime dispatch is still Octopus's own provisional plan until it actually happens - it can be moved or withdrawn beforehand, which could otherwise lead Predbat to make an irreversible decision (e.g. an early force-export) in anticipation of a cheap recharge that never occurs. The stricter levels below exist for anyone who has been caught out by this; the default matches Predbat's long-standing behaviour so nobody's plan changes unless they opt in. | ||
| Note that a slot's start time passing is not itself confirmation - Octopus can still revoke a dispatch that has technically started but where the car never actually drew power, so none of these levels trust a slot on clock time alone: | ||
| - `planned` (default) - trusts every daytime dispatch the moment Octopus plans it, with no confirmation at all - the behaviour Predbat has always used. Choose this if you'd rather the house battery plan ahead of a provisional slot (e.g. export in anticipation of it) than wait for any confirmation, and are comfortable with the plan occasionally acting on a slot Octopus later moves or withdraws. | ||
| - `none` - only the fixed 23:30-05:30 window is trusted as cheap for battery planning; every daytime dispatch is left at the normal rate until Octopus reports it as completed. |
There was a problem hiding this comment.
(Written by Claude, working with @chalfontchubby.)
Correct - the docs were wrong. none sets trusted = False whatever confirmed says, so a completed daytime dispatch stays at the normal rate too.
Reworded, and made the point positively rather than leaving it implied, since none is what someone picks when they specifically want nothing trusted:
none- only the fixed 23:30-05:30 window is trusted as cheap for battery planning. Every daytime dispatch is left at the normal rate, including one Octopus has already reported as completed - this is the strictest setting, and the house battery never plans around a dynamic slot at all.
Not sure this makes sense as completed is strictly after the slot finishes? |
7d94d81 to
5f8748b
Compare
|
(Written by Claude, working with @chalfontchubby.) You're right, and it's worse than a wording problem - Dropped it. The ladder is now:
One consequence worth flagging: Docs updated, including a note on why there's no completed-only level so this doesn't get proposed again. Tests 31/32/35/36/40 reworked onto |
…w sensor (#4482, #4516) Octopus Intelligent daytime dispatch slots are provisional: Octopus can move or withdraw one before it happens, and allocates them from its own assumption of what the car needs, since it cannot see the car's real SoC. Predbat treats every granted slot as guaranteed cheap for the house battery, so it can make an irreversible decision - an early force export, or deferring a charge - in anticipation of a window that then disappears, or import during a slot the car never uses, which Octopus bills at the full peak rate. Two independent gates, which a future out-of-window slot must both clear: - switch.predbat_octopus_intelligent_limit_future_slots (default Off) - does the car's own remaining SoC requirement still want this slot? Requires octopus_intelligent_consider_full, and warns at startup if enabled without it. - select.predbat_trust_future_dynamic_iog_slots (default "planned") - will Octopus honour it? "none" trusts no dynamic slot; "completed" requires Octopus's own metered record; "started" also accepts a slot corroborated by car_charging_now; "planned" is the long-standing trust-everything behaviour. The fixed 23:30-05:30 window is never gated - it is cheap by tariff structure, not by dispatch. Both defaults preserve today's behaviour, so an upgrade changes no plan until a user opts in. This reworks the design of the closed PR #4885 onto current main. That PR was rejected on the grounds that distrusting a slot costs a house-battery discharge into the car while waiting on a sensor, and that "it takes 5-10 minutes based on a sensor to figure out if the car is really charging". Measured on a live system since: a watched car_charging_now change reaches a register write in 17-40s (update_time_loop polls every 15s, so it does not wait for the 5-minute scheduled cycle), and the inverter acts on it 36-65s later - so under two minutes end to end from a locally reported charging state, not 5-10. The second objection was false positives: inferring "the car is charging" from a load signature could take a kettle for the car. Hence the sensor gate here - "started" requires a real charger/car-reported car_charging_now for that car, and degrades to "completed" per car without one, rather than guessing. #4948 has since landed the dispatch-timeline diagnostic that PR also carried, so only the trust logic is new. Two details worth knowing: car_charging_now confirms a 30-minute settlement slot as a "trusted streak" rather than per reading. get_arg() re-samples live state on each replan instead of consuming the edge that woke it, so a charging pulse shorter than the wakeup-to-read latency can be missed entirely rather than merely seen late. A guard-passing positive read therefore starts a streak that rolls forward through following slots until an explicit False reading, which also tolerates a charger toggling at a settlement boundary - observed live as a readiness sensor flipping Charging->Ready->Charging within two minutes, negotiation noise rather than a real stop. The guard applies only to streak starts, so a reading in a slot's last few minutes cannot confirm the slot after it; a cap bounds how far a streak rolls without fresh evidence, so a missed negative edge degrades to untrusted rather than silently behaving as "planned". Both constants are judgement calls, not measured. Rejecting a slot actively restores rate_max_base rather than merely declining to discount it, because on a real Intelligent tariff fetch_octopus_rates() can deliver the discounted rate directly from the feed. exclude_dynamic_io_slots() does the same for a feed-side discount with no matching dispatch entry, consuming the trust decisions rate_add_io_slots() just made so the two cannot disagree. Both touch only minutes from now onwards: an elapsed minute records what the tariff actually charged, and rewriting it would inflate today_cost() without changing a plan that never looks behind minutes_now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
5f8748b to
361cbfe
Compare
|
I found a concrete order-dependent failure, although the original 30-minute At the default 30-minute interval, Partial overlap does expose the problem with the supported
The planned slot claims minutes 855-869 in A direct unconfirmed-then-confirmed duplicate also reproduces the original gate-ordering issue, though current fetch ordering prevents that input. So the exact rationale should be narrowed, but Analysis and reproduction performed with OpenCode using OpenAI GPT-5.6 Sol. |
(Written by Claude, working with @chalfontchubby.)
Supersedes #4885, which was closed on two objections. Both are addressed here — the first with measurements rather than argument.
This is a defensive change. It trades away some upside to remove a downside: distrusting a slot that would have materialised costs a cheap window you could have used, but trusting one that doesn't costs real money — either an irreversible decision (an early force-export) made around a window that vanishes, or an import billed at full peak. Anyone who prefers the upside stays on the default, which trusts everything exactly as today.
The objections, answered
"It takes 5-10 minutes based on a sensor to figure out if the car is really charging."
Measured on a live system (Sigenergy + Hypervolt + VW ID.3), from the Predbat log:
car_charging_nowchanges → Predbat writes to the inverterThe 5-minute figure assumes the scheduled
run_time_loopcycle. It doesn't apply: a watched entity setsupdate_pending, andupdate_time_looppolls that every 15 seconds and replans out of cycle. I had this wrong myself until I measured it.A trigger routed via the Octopus app (a boost) adds ~1:35-1:56 of cloud round-trip, but that's upstream of Predbat either way.
"It might not detect it, or falsely detect say a kettle as the car."
Agreed — so
startednow requires a realcar_charging_nowsensor for that car: a charger/car-reported signal, never inferred from a load signature, so there's no kettle ambiguity. Without one it falls back tocompleted, decided per car, with a startup warning.What it does
Two independent gates a future out-of-window slot must both clear. They answer different questions and can be used together or separately:
switch.predbat_octopus_intelligent_limit_future_slots(default Off) — does the car still need this slot? Requiresoctopus_intelligent_consider_full; warns at startup if enabled without it.select.predbat_trust_future_dynamic_iog_slots(defaultplanned) — will Octopus honour it?planned— trust every dynamic slot. Today's behaviour.completed— require Octopus's own metered record.started— also accept a slotcar_charging_nowcorroborates, without waiting for that record.none— trust nothing outside the fixed window.The fixed 23:30-05:30 window is never gated — it's cheap by tariff structure, not by dispatch.
Both defaults preserve current behaviour. A fresh install or upgrade sees no plan change until someone opts in, per your condition last round.
On Predbat.com the gateway auto-wires
car_charging_nowfrom the charger's own OCPP session state, sostartedworks there too. Gateway users withgateway_evc_controldon't get that sensor (it's omitted to avoid a feedback loop) and fall back tocompleted— but if Predbat is driving the charger then Octopus isn't dispatching it, so there are no dynamic slots to gate anyway.Related issues
Several open issues describe this same exposure from different angles. Flagging them together since some may be answerable by this, and others are explicitly out of scope:
trust_future_dynamic_iog_slotsanswers directly.startedorcompletedis the intended answer; worth the reporter trying it.ignore_unpluggedcovers at fetch time.none/completedwould also stop the house battery relying on them, so it may help in practice.started, for installs with nocar_charging_now. Note that issue assumestrust_future_dynamic_iog_slotsalready exists on main — it didn't until now, so its premise is finally true. The load-confirmed half is deliberately not done here: inferring charging from load is exactly the kettle risk, which is why this requires a real sensor instead.measurementsAPI. Not built. That would turn the cost case here from anecdote into a number, and would validate whichever trust level a user picks.On #4482
limit_future_slotsaddresses #4482's failure mode — the house battery no longer defers charging into slots outside what the car still needs — but it is not the implementation that issue asks for. It reuses thecar_charging_slotscap rather than computing remaining EV energy fromcar_charging_soc/battery_size/limitdirectly, and so depends onconsider_full, which #4482 explicitly notes isn't sufficient on its own. So this mitigates it rather than closing it, and I've left the issue open for the reporter to judge.The structure is a single predicate per slot, so the stronger version builds on this rather than replacing it — #4482's own SoC calculation would swap out how the expected-slot set is derived. Time-bounded trust ("trust a slot starting within the next hour or so") would be another clause on the same decision. Out of scope here.
Notes for review
car_charging_nowconfirms a whole 30-minute settlement slot as a "trusted streak", not a single reading.get_arg()re-samples live state on each replan rather than consuming the edge that woke it, so a short pulse can be missed entirely rather than merely seen late. A streak rolls forward until an explicit "not charging" reading, which also tolerates a charger toggling at a settlement boundary — observed live as a readiness sensor flipping Charging→Ready→Charging within two minutes, negotiation noise rather than a real stop. A start guard stops a reading in a slot's closing minutes confirming the next slot; a 4-slot cap bounds how far a streak rolls without fresh evidence, so a missed negative edge degrades to untrusted rather than silently behaving asplanned. Both constants are judgement calls, not measured — happy to tune.rate_max_baserather than just declining to discount it, because on a real Intelligent tarifffetch_octopus_rates()can deliver the discounted rate straight from the feed.exclude_dynamic_io_slots()handles that feed-side case, consuming the same trust decisions so the two can't disagree. Both touch only future minutes — an elapsed minute records what the tariff actually charged, and rewriting it would inflatetoday_cost()without changing a plan that never looks behindminutes_now.Closes #4516. Addresses #4482.
Tests: 26 new/reworked cases across
test_rate_add_io_slots.py,test_fetch_config_options.pyand a newtest_car_charging_now_confirmed_slots.py. Full quick suite and pre-commit green.