Loadpoint: fix fast charging phase scaling#27972
Merged
andig merged 1 commit intoevcc-io:masterfrom Mar 7, 2026
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
TestFastChargingCircuitPhaseScaling, thefor _, tc := range tcloop closes over the loop variable int.Run, which can cause all subtests to use the last table entry; add atc := tcinside the loop beforet.Runto capture the value per iteration. - The test mutates the global
Voltagevariable without restoring it, which can lead to test ordering issues; consider saving the previous value and deferring its restoration inside the test. - The
4140constant used for the 3‑phase minimum power edge cases in the test seems derived fromVoltage * minCurrent * 3; computing it dynamically instead of hardcoding would make the test more robust to future changes inVoltageorminCurrent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `TestFastChargingCircuitPhaseScaling`, the `for _, tc := range tc` loop closes over the loop variable in `t.Run`, which can cause all subtests to use the last table entry; add a `tc := tc` inside the loop before `t.Run` to capture the value per iteration.
- The test mutates the global `Voltage` variable without restoring it, which can lead to test ordering issues; consider saving the previous value and deferring its restoration inside the test.
- The `4140` constant used for the 3‑phase minimum power edge cases in the test seems derived from `Voltage * minCurrent * 3`; computing it dynamically instead of hardcoding would make the test more robust to future changes in `Voltage` or `minCurrent`.
## Individual Comments
### Comment 1
<location path="core/loadpoint_phases_test.go" line_range="484" />
<code_context>
}
+
+func TestFastChargingCircuitPhaseScaling(t *testing.T) {
+ Voltage = 230
+
+ tc := []struct {
</code_context>
<issue_to_address>
**issue (testing):** Reset global `Voltage` after the test to avoid leaking state into other tests
This test mutates the package-level `Voltage` and never restores it, which can cause order-dependent test failures elsewhere. Capture the original value and defer restoring it, e.g. `oldVoltage := Voltage; Voltage = 230; defer func() { Voltage = oldVoltage }()` at the start of the test.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
mfuchs1984
force-pushed
the
fix/fast_charging_circuit_phase_scaling
branch
3 times, most recently
from
6db4166 to
34534f8
Compare
Member
|
I didn't check the tests, but the implementation looks perfectly sound. It would be great if we could get a test confirmation before merging. Maybe @eliaslecomte? |
mfuchs1984
force-pushed
the
fix/fast_charging_circuit_phase_scaling
branch
from
34534f8 to
49782c2
Compare
mfuchs1984
force-pushed
the
fix/fast_charging_circuit_phase_scaling
branch
from
49782c2 to
b4da4fb
Compare
Collaborator
Author
|
Accidentally pushed an invalid local test. Now it's ready. |
Collaborator
Author
|
Yeah, I can't test it this week unfortunately. @eliaslecomte would be great if you could build and run evcc with the changes from this PR. |
Contributor
|
Nice!
I'm looking at how I can best run this. For now I'm thinking to run it locally on my PC. I would need to copy my config (from home assistant) and do an export/restore backup through the UI. And then await some sun on this cloudy day. |
Member
|
Let‘s merge this for testing. The current implementation on master is clearly wrong and the associated bug is already much too old. Please let us know if this works as expected. |
Member
|
Thank you @mfuchs1984 great PR! |
Contributor
|
Now that it got merged: I'll install the (home assistant) nightly and test tomorrow. You can expect my test results in the evening. |
Collaborator
Author
|
I think you have to wait for the nightly tomorrow. |
Member
|
New nightly in 30min |
Contributor
|
Test results 🥳. Good news, first scenario tested and looks good. Setup
Scenario tested Even with a low site/grid limit, the charger should switch to 3p when there is enough solar. Before 
After Running the nightly. 
|
2 tasks
Collaborator
Author
|
Can you do the same later without surplus to verify it switches down? |
Contributor
|
After charging for half an hour, I noticed that it halted charging, while I believe that it could have lowered phases from 3 to 1, so that it could continue charging. Not sure if I have captured the full relevant log but this might help: |
Collaborator
Author
|
The log starts to late to understand what's going on. |
Contributor
|
I've retested the scenario of switching to 1 phase while it was already charging with 3 phases by limiting solar panels downwards. |
Collaborator
Author
|
@andig while the change itself seems fine, this might need a phase switching guard timer, like in PV mode, at least before switching up again. |
Collaborator
Author
|
Created #28174 as follow-up, preventing excessive repeated phase switching. |
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
3 participants
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.
This should fix the the fastCharging phase-switching logic to take the actual circuit load into account. This makes sure to take available surplus into account as well as circuit nesting.
lp.fastChargingnow used circuit.ValidatePower to determine if the minimum power for 3 phase charging is available. Instead of using the heuristic1.1*maxPower1p, I changed it to explicitly use minPower3p. Everything below will lead to scaling down.Fixes #24160
Replaces #27907
/cc @eliaslecomte
/cc @McFSR
/cc @joachimbaten