Loadpoint: add fast charging phase upscale delay#28174
Open
mfuchs1984 wants to merge 14 commits intoevcc-io:masterfrom
Open
Loadpoint: add fast charging phase upscale delay#28174mfuchs1984 wants to merge 14 commits intoevcc-io:masterfrom
mfuchs1984 wants to merge 14 commits intoevcc-io:masterfrom
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
fastCharging, the 3p scale-up path logs errors fromscalePhases(3)but does not return them (unlike the earlier implementation which returned errors fromscalePhasesIfAvailable), which may make diagnosing phase-switch issues harder; consider propagating these errors to the caller for better observability. - The 500 W hysteresis buffer for phase upscaling is hard-coded inside
fastCharging; consider extracting it into a named constant (or configuration) to make the rationale explicit and future tuning easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `fastCharging`, the 3p scale-up path logs errors from `scalePhases(3)` but does not return them (unlike the earlier implementation which returned errors from `scalePhasesIfAvailable`), which may make diagnosing phase-switch issues harder; consider propagating these errors to the caller for better observability.
- The 500 W hysteresis buffer for phase upscaling is hard-coded inside `fastCharging`; consider extracting it into a named constant (or configuration) to make the rationale explicit and future tuning easier.
## Individual Comments
### Comment 1
<location path="core/loadpoint.go" line_range="1281" />
<code_context>
// fastCharging scales to 3p if available and sets maximum current
func (lp *Loadpoint) fastCharging() error {
- if lp.hasPhaseSwitching() {
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting phase selection and timer-based scale-up into small helper functions so that `fastCharging` reads as a simple policy function instead of an inlined state machine.
You can keep the new behavior but reduce `fastCharging` complexity by pushing phase selection and timer handling into small helpers. That will remove the `waiting` flag, isolate branching, and make the function read as “policy” instead of “state machine”.
For example:
1. **Extract desired phase calculation**
```go
func (lp *Loadpoint) desiredFastChargingPhases() (phases int, powerLimit, minPower3p float64) {
phases = 3
minPower3p = currentToPower(lp.effectiveMinCurrent(), 3)
powerLimit = minPower3p
if lp.circuit != nil {
powerLimit = lp.circuit.ValidatePower(lp.chargePower, currentToPower(lp.effectiveMaxCurrent(), 3))
if powerLimit < minPower3p {
phases = 1
}
}
if lp.phasesConfigured != 0 {
phases = lp.phasesConfigured
}
return phases, powerLimit, minPower3p
}
```
2. **Extract timer-based scale-up into a helper that returns whether we’re still waiting**
```go
func (lp *Loadpoint) handlePhaseScaleUpFast(powerLimit, minPower3p float64) (waiting bool) {
const buffer = 500.0 // Watts
if lp.circuit != nil && powerLimit < minPower3p+buffer && lp.phasesConfigured != 3 {
lp.log.DEBUG.Printf(
"fast charging: staying at 1p (buffer), power limit %.0fW < %.0fW threshold",
powerLimit, minPower3p+buffer,
)
return true
}
if lp.circuit == nil || !lp.charging() {
lp.phaseTimer = elapsed
}
if lp.phaseTimer.IsZero() {
lp.log.DEBUG.Printf("fast charging: start phase %s timer", phaseScale3p)
lp.phaseTimer = lp.clock.Now()
}
lp.publishTimer(phaseTimer, lp.GetEnableDelay(), phaseScale3p)
if lp.clock.Since(lp.phaseTimer) >= lp.GetEnableDelay() {
if err := lp.scalePhases(3); err != nil {
lp.log.ERROR.Println(err)
}
return false
}
return true
}
```
3. **Use early returns in `fastCharging` and remove implicit `waiting` flow**
```go
func (lp *Loadpoint) fastCharging() error {
if !lp.hasPhaseSwitching() || !lp.phaseSwitchCompleted() {
return lp.setLimit(lp.effectiveMaxCurrent())
}
phases, powerLimit, minPower3p := lp.desiredFastChargingPhases()
activePhases := lp.ActivePhases()
// scale down: immediate
if phases == 1 && activePhases == 3 {
lp.log.DEBUG.Printf(
"fast charging: scaled to 1p to match %.0fW available circuit power",
powerLimit,
)
if err := lp.scalePhases(1); err != nil {
return err
}
lp.resetPhaseTimer()
return lp.setLimit(lp.effectiveMaxCurrent())
}
// scale up: buffered and delayed
if phases == 3 && activePhases == 1 {
waiting := lp.handlePhaseScaleUpFast(powerLimit, minPower3p)
if !waiting {
lp.resetPhaseTimer()
}
return lp.setLimit(lp.effectiveMaxCurrent())
}
// no phase change
lp.resetPhaseTimer()
return lp.setLimit(lp.effectiveMaxCurrent())
}
```
This preserves all added behaviors (circuit-aware phase choice, buffer, timer, logging) but:
- Splits responsibilities: phase decision vs. transition vs. timer logic.
- Removes the `waiting` flag from the main function.
- Makes `fastCharging` straightforward to read and test (three clear paths: scale down, scale up, no change).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
Author
|
Another option could be to introduce a static delay instead of using the configurable enable delay. |
Member
|
Seems scaling up enable delay should be fine give it can't cause any overload? |
andig
reviewed
Mar 14, 2026
| } | ||
|
|
||
| phases := 3 | ||
| minPower3p := currentToPower(lp.effectiveMinCurrent(), 3) |
Member
There was a problem hiding this comment.
That should actually be effective phases? But this is as-is. Not sure how we should treat 2p vehicles here. Stay with 3 to be on the safe side?
Member
There was a problem hiding this comment.
Move minPower3p into the if where it's used to unpollute function space
Collaborator
Author
There was a problem hiding this comment.
it is used in two places, this is why I put it there. Alternatively, I could call currentToPower(lp.effectiveMinCurrent(), 3) directly in both places.
andig
reviewed
Mar 14, 2026
| } | ||
| } | ||
|
|
||
| if lp.phasesConfigured != 0 { |
Member
There was a problem hiding this comment.
What does this do? If configured is 1p we might as well exit early on top? Is this covered by a test?
Member
There was a problem hiding this comment.
Especially, if this is 3, it will override the previous load management check! Seems tests don't catch this?
Collaborator
Author
There was a problem hiding this comment.
What does this do? If configured is 1p we might as well exit early on top? Is this covered by a test?
When a user manually selects 1p or 3p on the loadpoint, it makes sure that we scale phases to the selection. Yes, this overrides the previous load management check, but I think we should not auto-switch when the user disables it.
What about logging a warning when this happens?
Collaborator
Author
There was a problem hiding this comment.
Added a warning.
Especially, if this is 3, it will override the previous load management check! Seems tests don't catch this?
It is covered by the test cases "fixed 3p, 3p active" and ""fixed 3p, 1p active". They validate that the fixed phase setting overrides the circuit limit based phase switching.
andig
reviewed
Mar 14, 2026
|
|
||
| // scale down: immediate | ||
| if phases == 1 && activePhases == 3 { | ||
| lp.log.DEBUG.Printf("fast charging: scaled to 1p to match %.0fW available circuit power", powerLimit) |
Member
There was a problem hiding this comment.
Wondering if this needs a timer, too. But OT here.
Collaborator
Author
There was a problem hiding this comment.
Leave as is for now and wait for further feedback?
andig
reviewed
Mar 14, 2026
andig
reviewed
Mar 14, 2026
andig
reviewed
Mar 14, 2026
Collaborator
Author
|
👍 for the good comments |
Collaborator
Author
|
I tried to address the review comments as good as I was able to, please have another look 😊 |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
fastCharging, theif !lp.charging() { // scale immediately if no circuit or not charging }branch can never see the "no circuit" case becauselp.circuit == nilhas already returned earlier, so the comment is misleading and the condition name could be tightened (or the structure refactored) to better reflect the actual behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `fastCharging`, the `if !lp.charging() { // scale immediately if no circuit or not charging }` branch can never see the "no circuit" case because `lp.circuit == nil` has already returned earlier, so the comment is misleading and the condition name could be tightened (or the structure refactored) to better reflect the actual behavior.
## Individual Comments
### Comment 1
<location path="core/loadpoint.go" line_range="1297-1300" />
<code_context>
+ targetPhases = 1
+ }
+
+ if lp.phasesConfigured != 0 {
+ // user configured fixed phase count overrides automatic switching
+ targetPhases = lp.phasesConfigured
+ if lp.phasesConfigured == 3 && targetPhases == 1 {
+ lp.log.WARN.Printf("configured fixed phase count %dp prevents switching to 1p to match %.0fW available circuit power", lp.phasesConfigured, powerLimit)
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Condition `lp.phasesConfigured == 3 && targetPhases == 1` is unreachable after assigning `targetPhases = lp.phasesConfigured`.
As a result, the warning about preventing switching to 1p is never emitted. If you want to warn when the *auto-computed* target would have been 1p but a fixed 3p configuration overrides it, consider storing the pre-override value in a separate variable (e.g., `autoTargetPhases`) and checking that instead.
</issue_to_address>
### Comment 2
<location path="core/loadpoint.go" line_range="1283" />
<code_context>
- phases = 1
- lp.log.DEBUG.Printf("fast charging: scaled to 1p to match %.0fW available circuit power", powerLimit)
- }
+ if !lp.hasPhaseSwitching() || !lp.phaseSwitchCompleted() {
+ return lp.setLimit(lp.effectiveMaxCurrent())
+ }
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the phase decision and upscale handling into small helper functions to clarify intent, remove dead conditions, and keep `fastCharging` focused on orchestration.
You can keep all new behaviour but reduce complexity and fix a small logic issue with a few focused extractions.
### 1. Extract phase decision logic (and fix contradictory condition)
Right now:
```go
targetPhases := 3
minPower3p := currentToPower(lp.effectiveMinCurrent(), 3)
powerLimit := lp.circuit.ValidatePower(lp.chargePower, currentToPower(lp.effectiveMaxCurrent(), 3))
if powerLimit < minPower3p {
targetPhases = 1
}
if lp.phasesConfigured != 0 {
// user configured fixed phase count overrides automatic switching
targetPhases = lp.phasesConfigured
if lp.phasesConfigured == 3 && targetPhases == 1 {
lp.log.WARN.Printf("configured fixed phase count %dp prevents switching to 1p to match %.0fW available circuit power", lp.phasesConfigured, powerLimit)
}
}
```
The `lp.phasesConfigured == 3 && targetPhases == 1` condition is impossible after `targetPhases = lp.phasesConfigured`.
You can make the intent clearer by keeping both the *automatic* and the *final* decision in a small helper:
```go
func (lp *Loadpoint) determineTargetPhases(powerLimit, minPower3p float64) (autoPhases, targetPhases int) {
autoPhases = 3
if powerLimit < minPower3p {
autoPhases = 1
}
targetPhases = autoPhases
if lp.phasesConfigured != 0 {
if autoPhases == 1 && lp.phasesConfigured == 3 {
lp.log.WARN.Printf(
"configured fixed phase count %dp prevents switching to 1p to match %.0fW available circuit power",
lp.phasesConfigured, powerLimit,
)
}
targetPhases = lp.phasesConfigured
}
return autoPhases, targetPhases
}
```
And in `fastCharging`:
```go
minPower3p := currentToPower(lp.effectiveMinCurrent(), 3)
powerLimit := lp.circuit.ValidatePower(lp.chargePower, currentToPower(lp.effectiveMaxCurrent(), 3))
_, targetPhases := lp.determineTargetPhases(powerLimit, minPower3p)
activePhases := lp.ActivePhases()
```
This removes dead code, clarifies override semantics, and shortens `fastCharging`.
---
### 2. Isolate upscale state machine (and avoid cross-branch `waiting`)
The `waiting` flag currently crosses the entire function and is only relevant in the “1p → 3p” branch. You can isolate this logic and keep timer management inside a helper:
```go
func (lp *Loadpoint) handleScaleUpTo3p(powerLimit, minPower3p float64) (waiting bool, err error) {
powerLimit3p := 1.1 * minPower3p
if powerLimit < powerLimit3p && lp.phasesConfigured != 3 {
lp.log.DEBUG.Printf("fast charging: staying at 1p, power limit %.0fW < %.0fW threshold incl. buffer", powerLimit, powerLimit3p)
return false, nil
}
if !lp.charging() { // scale immediately if no circuit or not charging
lp.phaseTimer = elapsed
}
if lp.phaseTimer.IsZero() {
lp.log.DEBUG.Printf("fast charging: start phase %s timer", phaseScale3p)
lp.phaseTimer = lp.clock.Now()
}
lp.publishTimer(phaseTimer, lp.GetEnableDelay(), phaseScale3p)
if lp.clock.Since(lp.phaseTimer) >= lp.GetEnableDelay() {
if err := lp.scalePhases(3); err != nil {
return false, err
}
return false, nil
}
return true, nil
}
```
Then `fastCharging` becomes simpler and the `waiting` state is localized:
```go
func (lp *Loadpoint) fastCharging() error {
if !lp.hasPhaseSwitching() || !lp.phaseSwitchCompleted() {
return lp.setLimit(lp.effectiveMaxCurrent())
}
if lp.circuit == nil {
return lp.scalePhasesIfAvailable(3)
}
minPower3p := currentToPower(lp.effectiveMinCurrent(), 3)
powerLimit := lp.circuit.ValidatePower(lp.chargePower, currentToPower(lp.effectiveMaxCurrent(), 3))
_, targetPhases := lp.determineTargetPhases(powerLimit, minPower3p)
activePhases := lp.ActivePhases()
// scale down: immediate
if targetPhases == 1 && activePhases == 3 {
lp.log.DEBUG.Printf("fast charging: scaled to 1p to match %.0fW available circuit power", powerLimit)
if err := lp.scalePhases(1); err != nil {
return err
}
lp.resetPhaseTimer()
return lp.setLimit(lp.effectiveMaxCurrent())
}
// scale up: buffered and delayed
if targetPhases == 3 && activePhases == 1 {
waiting, err := lp.handleScaleUpTo3p(powerLimit, minPower3p)
if err != nil {
return err
}
if !waiting {
lp.resetPhaseTimer()
}
}
return lp.setLimit(lp.effectiveMaxCurrent())
}
```
This keeps all current behaviour (buffering, timer, configuration overrides) while:
- Removing a function-wide `waiting` flag.
- Localizing timer lifecycle management to the upscale path.
- Making `fastCharging` mainly orchestrate “decide phases / downscale / upscale / set limit”, which improves readability without changing semantics.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
FYI, I have this actual use case where the charging station is switching from 1p to 3p and back continuously in "Fast Charging" mode. |
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.
Follow-up to #27972
Problem:
The implementation in #27972 can lead to rapid repeated switching between 1p and 3p in case of load management restrictions.
This can be easily reproduces by using the demo config
demo_fast_circuit.yaml:
The result is switching back and forth within seconds.
This PR adresses the problem by:
-> together, these changes ensure that we only switch up under load management restrictions when there is sufficient power available for a while, keeping the charge process stable and reducing overpower conditions.
Using demo_fast_circuit.yaml again, the effect can be easily observed.
Limitations: