Repository navigation
fix(loadpoint): Charge now keeps charging across a charger reconnect - #1848
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2665af4ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !changed && current.sessionID == "" { | ||
| // A driver can report the session after the hardware. Check | ||
| // the next session against the last proven one. | ||
| current.sessionID = previous.sessionID |
There was a problem hiding this comment.
If the hardware identity returns before the session ID and remains session-less for two controller ticks, the previous session is forgotten. The first tick copies previous.sessionID here, but clears proofLost and adopts the current connection generation; the second tick therefore takes the default branch and overwrites the binding with an empty session ID. When a different session subsequently arrives, otherSession cannot detect it, and ObserveSample treats it as a first session proof without changing the generation, so the new session inherits the old Charge now power instead of requiring confirmation. Preserve the last proven session across every session-less tick; adding a second consecutive tick("easee:A", "", 4140, false) to TestLostProofCannotHandStartToALateNewSession exercises this gap.
Useful? React with 👍 / 👎.
A dropped charger socket, or a hardware identity that is missing for a
while, is not an unplug. Charge now keeps its power, and the binding keeps
the last proven charger. When the charger proves itself again, the same
hardware keeps Charge now; another serial or session ID pauses it and asks
for Start, as before. An observed unplug, Stop, a car that stops asking and
a config change still end it.
The site rig's s13 ("Charge now goes on after the charger returns") passes
now, so its known line goes.
Decision: #1788.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
c2665af to
8539edc
Compare
Problem
A charger's socket drop stopped Charge now. When an OCPP charger lost its
Wi-Fi and came back, still plugged in and with the same serial, FTW turned
Charge now into a saved 0 W pause with
manual_restore_unconfirmedandwaited for someone to press Start. No notice went out, since FTW ordered the
0 W itself. Fredrik's rule (#1772):
Charge now runs until Stop, unplug or the car stops asking; nothing else
stops it. Decision in #1788: the
same charger back without an observed unplug keeps Charge now, as
#1778 does after a restart.
Change
go/internal/loadpoint/controller_hold_restore.go,restoreManualHoldForSession:connection_generation) or a missing hardware identityno longer counts as a change. The hold keeps its power, and the binding
keeps the last proven charger (
proofLost).another serial, or a session ID that differs from the one before, pauses
it and asks for Start, as before.
change end it; a counter reset or new session on the same socket still
pauses it; FTW sends nothing while the socket is down; the fuse guard.
first proof, as before, now also across a socket drop.
session_state.go: one comment that described the old rule. No setting, notimer, no new state on disk.
The accepted risk, as in #1778: another car plugged in during the outage,
with the unplug unseen, gets Charge now until it is full, unplugged or
stopped. It costs money at worst.
Tests that encoded the old rule
Each now says the opposite, because each asserted the pause this PR removes:
TestManualStartWithoutHardwareCannotFollowAnotherSocket→TestManualStartBeforeHardwareProofSurvivesAReconnect. It asserted that anew socket pauses a Start made before any hardware proof. Now the Start
keeps 4 140 W across the new socket, binds to the first proof (
easee:A),keeps it across another socket, and pauses with
unconfirmedonly wheneasee:Bturns up.TestLostHardwareProofPausesOldStartButAllowsANewExplicitStart→TestLostHardwareProofKeepsStartUntilAnotherSessionProvesItself. Itasserted that a lost identity pauses the Start. Now the Start keeps
4 140 W while the identity is gone and when
easee:A/session-1returns;session-2on the same hardware pauses it; a new Start works and bindsto
session-2;easee:Bafter another loss pauses again.TestOCPPReconnectDoesNotUnplugOrResumeOldStart→TestOCPPReconnectIsNotAnUnplug(real OCPP server and charge point). Itasserted 0 W during the outage and after any reconnect. Now the hold keeps
its power during the outage (still no command, still
unavailable), thesame serial keeps it after the reconnect, and another serial pauses it.
The checks for no invented plug edge, a real unplug and a fresh Start stay.
New:
TestChargeNowSurvivesOCPPSocketBlip(cmd/ftw, real OCPP path) holdsthe rig's s13 criteria at unit level, at 11 040 W: the same charger, with
and without a tick during the outage, gets only 11 040 W after the return,
stays plugged in and raises no plug-in event; another serial pauses;
Availableafter the return clears the hold.The three rewritten tests, this one and the two from the review (below)
fail before this change and pass after.
go vetandgo testforinternal/loadpoint,cmd/ftw,internal/ocpp/...andtest/sitepass onthe final commit;
make verifypassed on the commit before the review fix.Site rig s13
s13-25("Charge now goes on after the charger returns", charger gone for10 s) was a known failure for #1788. It passes with this change, so its
knownline and the README mark go, as the rig's rules ask.make siterig SITERIG_SET=s13-25on this branch: pass in 4 min 40 s.Car at least 10 957 W from the drop to the end, 10 972 W over the last
60 s, no sample with the car unplugged.
From #1788, the earlier rig copy at 11 040 W with the proxy killed for 15 s:
paused, unconfirmedchargingNo time window
The hold has no timer: the same charger keeps Charge now after a drop of any
length. A window would end Charge now after a longer outage, which protects
no hardware and breaks the rule in #1772. Proof of the charger decides, as
#1778 does after a restart.
Review
An independent review (Claude Sonnet) found no blocker. Acted on:
cloud) could come back as
{A, ""}after a lost proof, and a later{A, session-2}then inherited Charge now. The binding now keeps the lastproven session through the gap, and a different non-empty session pauses
in every path (
otherSession). Test:TestLostProofCannotHandStartToALateNewSession, which fails before.TestChargeNowKeptAcrossReconnectSurvivesARestart: a kept holdleaves the saved record unchanged, so a restart after a drop restores
4 140 W. Fails before.
Accepted as they are, each within the risk in #1788:
ticks until its identity arrives; another serial then pauses it on its
first proof. The fuse guard applies throughout.
proof, as a Start before the first proof always has.
end Charge now; that is the stricter variant left out of Should a charger socket reconnect end Charge now? #1788.
The s13 rig run above used the commit before the session fix; that fix only
changes drivers that report session IDs, and OCPP reports none.
Order
On master after #1846, whose guard fails any test that dials with ocpp-go's
plain client; the new test uses its
chargersocket.ChargePoint16. #1837(draft) edits the same rig files on other lines.
Not in this PR
(
observeConnectionProofclearssocConfirmed), so "Charge now to 80 %"counts from an assumed level afterwards. Same question, separate change.
another car (Should a charger socket reconnect end Charge now? #1788) is not part of this change.
🤖 Generated with Claude Code