-
-
Notifications
You must be signed in to change notification settings - Fork 902
Loadpoint: split charging sessions when charger vehicle id changes #19479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @andig - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
// vehicle found or removed | ||
lp.setVehicleIdentifier(id) | ||
|
||
// create new charging session | ||
lp.createSession() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): Only create a session when a valid vehicle identifier is present
Move createSession() inside the if id != ""
block or add a guard so it only runs when the identifier is non-empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wäre setActiveVehicle()
nicht der bessere Ort um die Session zu splitten?
Ist es sinnvoll den Split nur bei geändertem Identifier durchzuführen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @premultiply,
der Kernpunkt ist: wir wollen wirklich nur dann eine neue Session starten bzw. eine bestehende beenden, wenn sich der Fahrzeug-Identifier ändert – und das passiert typischerweise in identifyVehicle()
, weil dort das Ergebnis der Erkennung („leer“, „Nutzer-Override“ oder eine echte ID) ankommt.
-
Warum nicht in
setActiveVehicle()
?- Das ist eher der UI-Override-Hook (wenn der User selbst das Fahrzeug wechselt).
- Unser Use-Case hier ist aber: das System bemerkt beim Polling einen geänderten Identifier (z. B. beim Abstöpseln/Rübersetzen).
- Würden wir es in
setActiveVehicle()
packen, fänden wir den „echten“ Erkennungs-Wechsel nicht ab, und hätten zwei Stellen, die sich um Session-Splitting kümmern müssten.
-
Split-Logik nur bei geändertem Identifier
Um false-positives (mehrfaches Stop/Clear/Create bei gleichem ID) zu vermeiden, sollte die Logik so aussehen:func (lp *Loadpoint) identifyVehicle() { id := lp.detectVehicleID() // pseudo old := lp.vehicleIdentifier() if id == old { return // keine Änderung → nichts tun } // altes Fahrzeug war gesetzt → Session beenden & clear if old != "" { lp.stopSession() lp.clearSession() } // neuen Identifier übernehmen lp.setVehicleIdentifier(id) // nur wenn wirklich ein neuer (nicht-leerer) Identifier vorliegt if id != "" { lp.createSession() } }
Damit stellen wir sicher, dass
- wir nur bei echtem Wechsel stoppen/clearen,
- wir nur bei vorhandenem Identifier eine neue Session anlegen,
- und alles zentral in der Erkennungs-Routine bleibt.
Klingt das für dich so sinnvoll? Wenn wir den manuellen Wechsel über setActiveVehicle()
ebenfalls supporten müssen, könnten wir dort genau dasselbe Muster nachziehen oder besser noch die Splitting-Funktion auslagern und sowohl in identifyVehicle()
als auch in setActiveVehicle()
aufrufen.
Fix #14995 (comment)