Repository navigation
fix(car): stop the car charge start time walking forward (#269) - #5120
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Two critical findings affecting charger control remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request fixes drifting car-charge start timestamps while preserving clamped planning calculations.
Changes:
- Stores original charging-window starts.
- Publishes stable starts and planned windows.
- Adds regression coverage for timestamp stability.
File summaries
| File | Summary |
|---|---|
apps/predbat/tests/test_car_charging_smart.py |
Adds regression coverage for stable timestamps. |
apps/predbat/plan.py |
Stores original starts. Critical finding (1 vote): long combined windows may trigger incorrect year handling in chargers. |
apps/predbat/output.py |
Publishes original starts. Critical finding (2 votes): starts older than 23 hours may be shifted into the next year by charger parsers. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ollover Both parsers bumped a window's year forward whenever its parsed start was more than 23h in the past, to handle a plan built just before New Year carrying Jan 1 windows. But a genuinely active long/flat-rate window that started well over 23h ago (its end still ahead of now) hit the same heuristic and got shifted a year forward, dropping out of should_charge_now()/_should_ev_charge_now() and stopping the charger mid-window (#269, Copilot review on #5120). Only bump the year when the window has also already finished (end < now) - the signature of a genuine rollover artefact, not an active window. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
(Posted by Rik; working with Claude.) Both Copilot findings on this PR are fixed in 47bba41: Ohme and GatewayMQTT could mistake a still-active window for a New Year rollover. Both parsers bump a parsed window's year forward whenever its The fix: only treat it as a rollover when the window has also already finished ( Added regression tests for a >23h-old but still-active window in both |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical New Year rollover defects remain in Ohme and Gateway, along with a moderate test-isolation issue.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
… now has itself rolled over Copilot review on #5120: at 00:15 on 1 January, a published 23:30-01:30 window parsed its Dec 31 start with now.year (the new year), putting it a full year in the future rather than the previous Dec 31 it actually meant. Neither existing correction branch caught this - the start wasn't "far in the past" (it was wrongly far in the future), and the elif's end<start bump only made it worse, landing the window almost a year ahead and excluding now entirely. Identical bug, independently duplicated, in gateway.py's _refresh_ev_windows() and ohme.py's refresh_car_windows(). Replaced both call sites' bespoke heuristics with a single new utils.localize_yearless_window(): each of start/end is independently anchored to whichever of now.year-1/now.year/now.year+1 places it closest to now, which gets the New Year straddle right as a side effect (start and end are always only hours apart, well inside the ~6-month anchoring tolerance) without a separate correction branch - the old elif turned out to be dead code once anchoring is done this way, so it's gone too. Added a dedicated test_localize_yearless_window.py, new regression cases in test_gateway.py/test_ohme.py for the after-rollover scenario, and rewrote test_gateway.py's old "year_boundary" test, which had been exercising a 30-hours-old-but-same-year window that was never actually a year-boundary case under the old heuristic's own terms. All new cases verified to fail against the prior code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
f0fe937 to
61b67a0
Compare
|
(Written by Claude, posted by @chalfontchubby.) A correction to the two replies I posted earlier today on the Ohme and gateway New Year threads. They described Now fixed in 61b67a0:
The branch is rebased onto current main and squashed into that one commit, so the commits cited in earlier replies (47bba41, f0fe937) no longer exist on it - the first commit on its own would also have reintroduced the 23-hour rollover bug for long windows. The PR description is rewritten to match. |
… Wallbox among the plan readers (#5120 review) An Intelligent dispatch already underway has its start trimmed to now by the Octopus component, so the comment's claim that Octopus slots keep their own start unchanged was wrong for those. The shared plan parser also has a new caller in Wallbox since the rebase. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
61b67a0 to
281a74a
Compare
plan_car_charging() clamps an in-progress window's start to minutes_now so the kWh and cost maths only count time the car can still charge for. That clamped value was also published as predbat.car_charging_start and in the car_charging_slot sensor's planned attribute, so the displayed start advanced by five minutes every cycle while the car charged. plan_car_charging() now records the window's real start as start_orig and publish_car_plan() shows that; all planning arithmetic still uses the clamped start. Octopus Intelligent slots carry no start_orig and fall back unchanged. The published start can now be hours, or for a long combined window more than a day, in the past. Ohme and the gateway EVC had their own year repair that treated any start over 23 hours old as a New Year rollover and pushed it a year forward, stopping the charger mid-window. Both now use utils.parse_car_plan_windows() / in_car_plan_window(), which myenergi and GivEnergy EVC already share: it rebuilds the missing year symmetrically around now with a 180-day margin, so a New Year window is anchored correctly whichever side of midnight it is read. It now also parses with the year in the string - strptime without one assumes 1900, which has no 29 February, so every charger dropped a leap-day window - and keeps the year shifts inside the try, as a stale 29 February entry has no counterpart in the neighbouring year. Tests: car_charging_start_stable_issue269 steps through one window and asserts the published start holds (restoring the dashboard state it publishes); car_plan_windows covers the helper directly, including New Year both ways, a January window read on 31 December, leap day and malformed entries; Ohme and gateway cover the same cases through the chargers on pinned dates. Fixes #269 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… Wallbox among the plan readers (#5120 review) An Intelligent dispatch already underway has its start trimmed to now by the Octopus component, so the comment's claim that Octopus slots keep their own start unchanged was wrong for those. The shared plan parser also has a new caller in Wallbox since the rebase. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
281a74a to
be0ccf0
Compare
springfall2008
left a comment
springfall2008
left a comment
There was a problem hiding this comment.
Approved. Moving Ohme and the gateway charger onto the shared parse_car_plan_windows() and the leap-day fix are good, and publishing start_orig cannot make a charger start early: the planner never moves a slot later than its window start. The charger and car plan test modules passed on a local merge with main before the rebase.
One thing to follow up, not a blocker. This stops the 5-minute drift from #269 but the start can still move:
- With
combine_charge_slotsoff (the default), low-rate windows are split into plan-interval chunks (find_charge_window(), thecharge_slot_splitbreak), sostart_origis the start of the current 30-minute chunk and the displayed start steps forward at each chunk boundary. - The window scan starts at today's midnight, so a 23:30-05:30 window reads as starting at 00:00 after midnight, not 23:30 as the description's example says.
car_charging_start_stable_issue269steps three 5-minute cycles inside one window, so it crosses neither.
(Review written by Claude on behalf of @springfall2008.)
This PR was written by Claude, working with me.
Refs #269, the oldest outstanding issue in the tracker, from November 2023. (Changed from "Fixes" at merge so the issue stays open: the start still moves at each low-rate window boundary, see the approval review.)
The problem
Once a car charging window is underway,
predbat.car_charging_startadvances by five minutes on every cycle, so the displayed "charging starts at" never settles - it just tracks the clock. The actual charge session is unaffected, which is why it went unnoticed for so long.Root cause
plan_car_charging()clamps a window's start tominutes_now:This is correct, and load-bearing. Everything below it treats
start→endas deliverable charging time -length = end - start, thenkwh = rate * hours. The plan is rebuilt from scratch every cycle against the car's current SoC, with no memory that a window already began, so without the clamp a 23:30–05:30 window still on the books at 03:00 would be credited with six hours of charging when only 2.5 remain.The bug is that
publish_car_plan()then reused that planning-internal value as the user-facing timestamp.Fix
plan_car_charging()records the window's real start asstart_orig;publish_car_plan()displays that for bothcar_charging_startand the slot sensor'splannedattribute. All kWh and cost arithmetic still uses the clamped start, so planning behaviour is unchanged. A window that is already underway now shows when it began (e.g. 23:30 when read at 02:00), while its kWh and cost cover only the part still to come.Octopus Intelligent slots never go through
plan_car_charging(), so they carry nostart_origand show their own start. That start is not fixed either: the Octopus component trims an in-progress dispatch's start to now on every poll, so an Intelligent dispatch already underway still shows a start that moves. This PR fixes Predbat's own planned windows only; doing the same for dispatches would mean carrying the original start through the Octopus component's trim.Effect on charger control
plannedalso drives real chargers: Ohme, myenergi, GivEnergy EVC, Wallbox and the gateway EVC read it back and checkstart <= now < end. The published start can now legitimately be hours - or, for a long combined window, more than a day - in the past. Ohme and the gateway had their own year repair that treated any start more than 23 hours old as a New Year rollover and pushed it a year forward, which would have stopped the charger mid-window.Both now use
utils.parse_car_plan_windows(), the helper myenergi and GivEnergy EVC already share. It rebuilds the plan's missing year symmetrically around now with a 180-day margin, so a long window is never mistaken for a rollover and a New Year window is anchored correctly whichever side of midnight it is read. It now also parses with the year in the string -strptimewithout a year assumes 1900, which has no 29 February, so every charger dropped a leap-day window.Testing
car_charging_start_stable_issue269stepsminutes_nowforward a cycle at a time inside one planned window and asserts the published start does not move. Reverting only the publisher prints['12:00:00', '12:05:00', '12:10:00']- the drift from the issue. It asserts the first window is at least 15 minutes long so it cannot go vacuous, and restores the dashboard state it publishes.car_plan_windowstestsparse_car_plan_windows()directly: New Year read before and after midnight, a long active window, a midnight crossing, leap day, and malformed or stale entries. The leap-day cases fail on main.control_midnight_after) and gateway (test_refresh_ev_windows_year_boundary,test_refresh_ev_windows_long_active_not_shifted) cover the same cases through the chargers, on pinned dates../run_all --quickandpre-commit run --all-filespass (exit 0).