Repository navigation
Phasenumschaltung series2: auf Zählerstrom 0 A und EVSE-Status B warten (#4289) - #4330
Open
Stefan3934 wants to merge 2 commits into
Open
Stefan3934 wants to merge 2 commits into
Stefan3934 wants to merge 2 commits into
Conversation
…en (openWB#4289) Seit openWB#3326 wartet perform_phase_switch nur auf den EVSE-Sollwert 0 A. Der steht sofort nach set_current(0) auf 0, noch bevor das Fahrzeug die Ladung beendet hat. CP und Schütz werden dann unter Last getrennt, einige Fahrzeuge (z. B. Audi A6 e-tron) gehen danach in einen Ladefehler. Außerdem brach jeder einzelne serielle Modbus-Fehler beim Rücklesen die Umschaltung ab. Jetzt wird vor dem CP-Trennen gepollt, bis der Ladepunktzähler auf allen Phasen < 0,5 A zeigt UND das EVSE-Statusregister 1002 nicht 3/4 (State C) meldet; "alle Phasen None" zählt nicht als 0 A. Busfehler beim Rücklesen werden nur geloggt, 0 A wird je Durchlauf erneut gesetzt. Ohne Bestätigung innerhalb von 20 Durchläufen wird abgebrochen, ohne GPIO-Aktion. Der CP wird im finally wieder verbunden. Dafür merkt sich Evse.get_plug_charge_state() den Rohstatus in state_number. Getestet auf series2 (EVSE-FW 18, MPM3PM) mit Audi A6 e-tron unter Last in beide Richtungen, siehe openWB#4289. Co-Authored-By: Claude Fable 5.1 <[email protected]>
Linked issuesChecked at commit 9567b58 #4289 [BUG]Phasenumschaltung Series2 seit #3326: CP wird ohne Wartezeit unter Last getrennt (EV meldet Ladefehler), serieller Modbus-Aussetzer bricht die Umschaltung ab · 3 of 3 met
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/modules/internal_chargepoint_handler/chargepoint_module.py" line_range="169" />
<code_context>
+ state = evse.state_number
+ log.debug(f"Phasenumschaltung LP{num}: Durchlauf {poll}, Ströme {currents} A, EVSE-Status {state}")
+ # keine Messwerte (alle None) gelten nicht als 0 A
+ if werte and max(werte) < self.PHASE_SWITCH_CURRENT_THRESHOLD_A and state not in (3, 4):
+ log.warning(f"Phasenumschaltung LP{num}: 0 A und EVSE-Status {state} nach {poll} Durchläufen"
+ f" / {time.monotonic() - start:.1f} s bestätigt, Ströme {currents} A.")
</code_context>
<issue_to_address>
**Missing readings permit unsafe switches**
When one or more phase readings are unavailable, including when all currents and powers are missing, `werte` filters out `None`, and `CounterState` can normalize wholly missing readings to zero, so the check passes without verified current on every phase and the phase-switch GPIO sequence disconnects the vehicle under unverified load.
Preserve missing-reading status through normalization and require a valid below-threshold current reading for every phase before allowing the switch.
Also at `packages/modules/internal_chargepoint_handler/chargepoint_module.py:170`.
</issue_to_address>
### Comment 2
<location path="packages/modules/internal_chargepoint_handler/chargepoint_module.py" line_range="169" />
<code_context>
+ state = evse.state_number
+ log.debug(f"Phasenumschaltung LP{num}: Durchlauf {poll}, Ströme {currents} A, EVSE-Status {state}")
+ # keine Messwerte (alle None) gelten nicht als 0 A
+ if werte and max(werte) < self.PHASE_SWITCH_CURRENT_THRESHOLD_A and state not in (3, 4):
+ log.warning(f"Phasenumschaltung LP{num}: 0 A und EVSE-Status {state} nach {poll} Durchläufen"
+ f" / {time.monotonic() - start:.1f} s bestätigt, Ströme {currents} A.")
</code_context>
<issue_to_address>
**Reverse current passes the zero check**
When a meter reports substantial negative current while the EVSE status is not 3 or 4, `max(werte)` compares signed readings, so a substantial negative current is below the threshold and the phase-switch GPIO sequence runs under load.
Compare the magnitude of each valid phase current with the threshold before allowing the switch.
</issue_to_address>
### Comment 3
<location path="packages/modules/internal_chargepoint_handler/chargepoint_module.py" line_range="186-191" />
<code_context>
else:
- raise Exception("Ladung konnte nicht gestoppt werden - Phasenumschaltung abgebrochen.")
- GPIO.output(gpio_cp, GPIO.HIGH) # CP off
- GPIO.output(gpio_relay, GPIO.HIGH) # 3 on/off
- time.sleep(5)
- GPIO.output(gpio_relay, GPIO.LOW) # 3 on/off
- time.sleep(5)
- GPIO.output(gpio_cp, GPIO.LOW) # CP on
+ raise Exception("Fahrzeug hat die Ladung nicht beendet (Zählerstrom > 0 oder EVSE-Status C)"
+ " - Phasenumschaltung abgebrochen.")
+ try:
+ GPIO.output(gpio_cp, GPIO.HIGH) # CP off
+ GPIO.output(gpio_relay, GPIO.HIGH) # 3 on/off
+ time.sleep(5)
+ GPIO.output(gpio_relay, GPIO.LOW) # 3 on/off
+ time.sleep(5)
+ finally:
+ GPIO.output(gpio_cp, GPIO.LOW) # CP on - in jedem Fall wieder verbinden (#4037)
time.sleep(1)
self.old_phases_in_use = phases_to_use
</code_context>
<issue_to_address>
**Relay failure leaves phase state inconsistent**
When a GPIO operation or delay raises after the relay has changed state, the `finally` block reconnects CP without ensuring the relay is reset, while `old_phases_in_use` remains unchanged; charging can resume with the hardware phase configuration different from the recorded configuration.
On relay-sequence errors, restore or confirm the relay configuration and synchronize `old_phases_in_use` before reconnecting CP.
</issue_to_address>
### Comment 4
<location path="packages/modules/internal_chargepoint_handler/chargepoint_module.py" line_range="186-191" />
<code_context>
else:
- raise Exception("Ladung konnte nicht gestoppt werden - Phasenumschaltung abgebrochen.")
- GPIO.output(gpio_cp, GPIO.HIGH) # CP off
- GPIO.output(gpio_relay, GPIO.HIGH) # 3 on/off
- time.sleep(5)
- GPIO.output(gpio_relay, GPIO.LOW) # 3 on/off
- time.sleep(5)
- GPIO.output(gpio_cp, GPIO.LOW) # CP on
+ raise Exception("Fahrzeug hat die Ladung nicht beendet (Zählerstrom > 0 oder EVSE-Status C)"
+ " - Phasenumschaltung abgebrochen.")
+ try:
+ GPIO.output(gpio_cp, GPIO.HIGH) # CP off
+ GPIO.output(gpio_relay, GPIO.HIGH) # 3 on/off
+ time.sleep(5)
+ GPIO.output(gpio_relay, GPIO.LOW) # 3 on/off
+ time.sleep(5)
+ finally:
+ GPIO.output(gpio_cp, GPIO.LOW) # CP on - in jedem Fall wieder verbinden (#4037)
time.sleep(1)
self.old_phases_in_use = phases_to_use
</code_context>
<issue_to_address>
**CP interruption overlaps phase switching**
When a single update requests both a phase switch and a positive CP-interruption duration, `UpdateState.update_state()` starts the phase-switch thread and then can start a CP-interruption thread in the same update. Both threads drive the same CP pin; the phase-switch `finally` can reconnect the vehicle while the interruption is still active, and the interruption can likewise change CP during relay switching, leaving the interruption or phase-switch operation in an unintended state.
Serialize the two operations or explicitly coordinate CP ownership so neither thread can change CP while the other operation is active.
</issue_to_address>
### Comment 5
<location path="packages/modules/common/evse.py" line_range="60" />
<code_context>
1000, [ModbusDataType.UINT_16]*3, unit=self.id)
# remove leading zeros
self.evse_current = int(raw_set_current)
+ self.state_number = int(state_number)
log.debug("Gesetzte Stromstärke EVSE: "+str(self.evse_current) +
", Status: "+str(state_number)+", Modbus-ID: "+str(self.id))
</code_context>
<issue_to_address>
**Concurrent status read goes stale**
When the regular poll and phase-switch thread overlap, and a newer State C read is overwritten by the older call's State B result while the meter reports below-threshold current, a normal chargepoint poll and the phase-switch thread both call `get_plug_charge_state()` on the same EVSE; the older call can overwrite `state_number` after the newer call records State C, so the phase-switch thread reads stale State B and proceeds to disconnect CP.
Return the raw status from the getter and make the phase-switch decision from that call's local result rather than a shared mutable field.
Also at `packages/modules/internal_chargepoint_handler/chargepoint_module.py:166`.
</issue_to_address>
### Comment 6
<location path="packages/modules/internal_chargepoint_handler/chargepoint_module.py" line_range="158-159" />
<code_context>
- _, _, evse_current = evse.get_plug_charge_state()
- if evse_current == 0:
- break
+ if meter is None:
+ raise Exception("Kein Zähler gefunden, Zählerstrom nicht prüfbar - Phasenumschaltung abgebrochen.")
+ evse.set_current(0) # Schreibfehler -> Abbruch: ohne 0-A-Vorgabe wird nicht geschaltet (#3326)
+ for poll in range(1, self.PHASE_SWITCH_METER_POLLS + 1):
</code_context>
<issue_to_address>
**Startup loses unmetered chargepoint**
When a chargepoint has no detectable meter and starts while system uptime is under 180 seconds, `perform_phase_switch(1)` raises before switching, and `InternalChargepointHandler.__init__` catches the exception while setting `cp0` to `None`; the chargepoint is therefore unavailable after startup.
Skip the automatic phase switch when no meter is configured, or preserve chargepoint initialization while safely reporting that the switch was not performed.
</issue_to_address>
Comment on lines
+186
to
+191
| GPIO.output(gpio_relay, GPIO.HIGH) # 3 on/off | ||
| time.sleep(5) | ||
| GPIO.output(gpio_relay, GPIO.LOW) # 3 on/off | ||
| time.sleep(5) | ||
| finally: | ||
| GPIO.output(gpio_cp, GPIO.LOW) # CP on - in jedem Fall wieder verbinden (#4037) |
There was a problem hiding this comment.
🟡 Medium · Relay failure leaves phase state inconsistent
When a GPIO operation or delay raises after the relay has changed state, the finally block reconnects CP without ensuring the relay is reset, while old_phases_in_use remains unchanged; charging can resume with the hardware phase configuration different from the recorded configuration.
On relay-sequence errors, restore or confirm the relay configuration and synchronize old_phases_in_use before reconnecting CP.
Prompt for AI agents
In `packages/modules/internal_chargepoint_handler/chargepoint_module.py` at lines 186-191:
**Relay failure leaves phase state inconsistent**
When a GPIO operation or delay raises after the relay has changed state, the `finally` block reconnects CP without ensuring the relay is reset, while `old_phases_in_use` remains unchanged; charging can resume with the hardware phase configuration different from the recorded configuration.
On relay-sequence errors, restore or confirm the relay configuration and synchronize `old_phases_in_use` before reconnecting CP.
Comment on lines
+186
to
+191
| GPIO.output(gpio_relay, GPIO.HIGH) # 3 on/off | ||
| time.sleep(5) | ||
| GPIO.output(gpio_relay, GPIO.LOW) # 3 on/off | ||
| time.sleep(5) | ||
| finally: | ||
| GPIO.output(gpio_cp, GPIO.LOW) # CP on - in jedem Fall wieder verbinden (#4037) |
There was a problem hiding this comment.
🟡 Medium · CP interruption overlaps phase switching
When a single update requests both a phase switch and a positive CP-interruption duration, UpdateState.update_state() starts the phase-switch thread and then can start a CP-interruption thread in the same update. Both threads drive the same CP pin; the phase-switch finally can reconnect the vehicle while the interruption is still active, and the interruption can likewise change CP during relay switching, leaving the interruption or phase-switch operation in an unintended state.
Serialize the two operations or explicitly coordinate CP ownership so neither thread can change CP while the other operation is active.
Prompt for AI agents
In `packages/modules/internal_chargepoint_handler/chargepoint_module.py` at lines 186-191:
**CP interruption overlaps phase switching**
When a single update requests both a phase switch and a positive CP-interruption duration, `UpdateState.update_state()` starts the phase-switch thread and then can start a CP-interruption thread in the same update. Both threads drive the same CP pin; the phase-switch `finally` can reconnect the vehicle while the interruption is still active, and the interruption can likewise change CP during relay switching, leaving the interruption or phase-switch operation in an unintended state.
Serialize the two operations or explicitly coordinate CP ownership so neither thread can change CP while the other operation is active.- Prüfung in _vehicle_stopped_charging(): jede Phase braucht einen gültigen Messwert mit abs(i) < 0,5 A, None zählt nicht als 0 A, Vorzeichen egal; bestätigt wird nur bei EVSE-Status A/B (5 = Fehler ist keine Bestätigung) - Evse.get_state_number() liefert den Rohstatus der eigenen Lesung statt eines geteilten Attributs (der reguläre Zyklus liest parallel) - ohne gefundenen Zähler kein Abbruch mehr (sonst fiele LP0 beim Boot weg), sondern WARNING und Prüfung nur über den EVSE-Status - chargepoint_module_test.py: 13 Fälle für die Entscheidung, 4 für die Schleife (Restlast, Busfehler, Abbruch ohne GPIO, ohne Zähler) Co-Authored-By: Claude Fable 5.1 <[email protected]>
Author
|
Danke für die Review. Vier der sechs Punkte habe ich im zweiten Commit umgesetzt, zwei betreffen Verhalten, das Umgesetzt
Dazu Nicht geändert
Flake8 und pytest laufen lokal durch. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4289
Problem
Seit #3326 wartet
perform_phase_switch()nur darauf, dass der EVSE-Sollwert (Register 1000) 0 A meldet. Der steht sofort nachset_current(0)auf 0, noch bevor das Fahrzeug die Ladung tatsächlich beendet hat. CP und Schütz werden dann unter Last getrennt; einige Fahrzeuge (bei mir Audi A6 e-tron) gehen danach in einen Ladefehler und laden erst nach Ab- und Anstecken wieder. Zusätzlich brach jeder einzelne Modbus-Fehler auf dem gemeinsamen RS485-Bus (EVSE + Zähler) die Umschaltung ab, mit falscher Phasenzahl im Datenmodell als Folge.Änderung
Fassung aus der Diskussion in #4289 (@seaspotter) plus zwei Ergänzungen:
None" (z. B. B23 übercheck_nan()) zählt nicht als 0 A, sondern als fehlende Messung.try/finally, der CP wird in jedem Fall wieder verbunden (wie in CP nach Fehler/Abbruch bei Phasenumschaltung und CP-Unterbrechung immer wieder verbinden #4037 vorgesehen).Evse.get_plug_charge_state()merkt sich den Rohstatus zusätzlich instate_number(analog zuevse_current), damit die Schleife ihn ohne zweite Registerlesung bekommt.Logs: je Durchlauf DEBUG mit Strömen und Status, bei Bestätigung und nach der Umschaltung WARNING mit Dauer (landet im main.log unter dem Thread "perform phase switch").
Test
openWB series2, EVSE-Firmware 18, MPM3PM-Zähler, Audi A6 Avant e-tron, beide Richtungen unter voller Last per "Anzahl Phasen" im Sofortladen (Details und Logs in #4289):
Kein Busfehler, kein Abbruch, kein Ladefehler am Fahrzeug.
🤖 Generated with Claude Code
Summary by Sourcery
Harden EVSE phase switching by confirming that vehicle charging has stopped before changing phases and by recovering safely from transient bus errors.
Bug Fixes:
Enhancements:
Tests: