Amperfied connect.solar: fix phase switching#29126
Amperfied connect.solar: fix phase switching#29126premultiply wants to merge 8 commits intomasterfrom
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
phases1p3p, the pre-switch current reduction viawb.set(ampRegAmpsConfig, 60)does not updatewb.current, so subsequent logic that relies onwb.current(e.g.,Enable(true)restoring the last current) may behave inconsistently; consider updatingwb.currentunder the mutex when forcing the 6 A minimum. - Access to
wb.phasesis now guarded by the mutex when it’s written and when the device returns 0 ingetPhases, but not when the device returns a non-zero value; for consistency and to avoid subtle races, you might want to wrap all reads/writes ofwb.phaseswithwb.mu.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `phases1p3p`, the pre-switch current reduction via `wb.set(ampRegAmpsConfig, 60)` does not update `wb.current`, so subsequent logic that relies on `wb.current` (e.g., `Enable(true)` restoring the last current) may behave inconsistently; consider updating `wb.current` under the mutex when forcing the 6 A minimum.
- Access to `wb.phases` is now guarded by the mutex when it’s written and when the device returns 0 in `getPhases`, but not when the device returns a non-zero value; for consistency and to avoid subtle races, you might want to wrap all reads/writes of `wb.phases` with `wb.mu`.
## Individual Comments
### Comment 1
<location path="charger/amperfied.go" line_range="249-251" />
<code_context>
curr := uint16(10 * current)
+ wb.mu.Lock()
+ inSwitch := time.Now().Before(wb.phaseSwitchEnd)
+ wb.mu.Unlock()
+
b := make([]byte, 2)
</code_context>
<issue_to_address>
**issue (bug_risk):** Current is updated even when the write fails during phase switching, which can desynchronize cached state from the device on unrelated errors.
In `MaxCurrentMillis`, when `inSwitch` is true, any error from `WriteMultipleRegisters` is ignored and `wb.current` is still updated. This makes sense for expected "reject during phase switch" errors, but it also hides other failures (e.g. connection issues) and can desync the cache from the device if the write didn’t actually succeed. Please at least log errors in the `inSwitch` path, or, if possible, distinguish expected phase-switch errors from other failures so that unexpected ones still propagate.
</issue_to_address>
### Comment 2
<location path="charger/amperfied.go" line_range="207-212" />
<code_context>
enabled := cur != 0
if enabled {
+ wb.mu.Lock()
wb.current = cur
+ wb.mu.Unlock()
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The read-before-lock pattern in Enabled can lead to `wb.current` reflecting a stale value under concurrent updates.
Because `cur` is read and `enabled` computed before taking the lock, a concurrent `MaxCurrentMillis` call could change the configured current between the read and the `wb.current` assignment, causing `wb.current` to lag behind the actual configuration. If you need `wb.current` to always reflect the latest configuration, consider either taking the mutex for both the Modbus read and the assignment, or treating `wb.current` as a logical target and not updating it here. Otherwise, please confirm that this possible drift is acceptable.
```suggestion
enabled := cur != 0
// wb.current reflects the logical target current configured via MaxCurrentMillis;
// do not update it here based on a potentially stale Modbus read.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
... edited: |
|
Currently testing automatic phase switching in PV mode. I’ll keep an eye on the logs for the next two days while the car is plugged in and let you know how it goes. |
|
This does not work as expected. I encountered logic errors and odd switching behavior, which I tried to cover with the attached logs. The trace log has been filtered to the load point and amperfied areas. 
evcc-20260416-181343-trace.log |
|
I'm also encountering issues. I only created a debug log, i hope you can work with that. To hopefully make it understandable I structured the description in 3 categories: User interaction: Action triggered by me in the EVCC frontend Test Setup:Loadpoint name: Carport Manual phase switch from 1P to 3PStep 1: Step 2:
EVCC behaviour after phase switch: 3P charging Manual phase switch from 3P to 1PUser interaction: change "Phases" setting for loadpoint "Carport" from "3-phasig" to "1-phasig"
EVCC behaviour: Expects 1P charging, recognizes 3P charging and again triggers phase switch From that point on it runs into a loop. Evcc triggers phase switch from 3P to 1P, wallbox switches phases from 3P to 1P, wallbox immediatley switches back again from 1P to 3P, wallbox starts charging. EVCC initiates phase switch again... --> Result: Phase switch from 3P to 1P not successfull The key would be to figure out why when switching from 3P to 1P immediatley after the first phase switch a second one is triggered. I hope that helps, I did my best. |
The whole log file never shows any 3p charging. |
|
Maybe I triggered the manual phase switch from 3P back to 1P too fast (within the EVCC update interval, 30s configured in my case). New log attached, Similar behaviour |
|
I can confirm same behaviour as reported by @7BC Phase switching did not work. Only when triggered manually. Hope the log files are sufficient to support the development process. |
|
@jwiesenm 1P to 3P always works for me but 3P to 1P does not. |
This tries to fix phase-switching on Amperfied
connect.solarwallboxes.Writes to the current register (261) during an ongoing phase switch were being rejected by the charger. This PR synchronizes evcc's current updates with the wallbox's phase-switch state.
Changes
0to register504on startup (only when 1p3p phase-switching is enabled, i.e.connect.solar), so the wallbox no longer blocks writes with its own grace period. The waiting period is maintained within evcc's own timers instead.503after the switch is triggered and, while that window is active, suppress modbus errors on writes to261.wb.currentis still tracked locally so interim setpoint changes are not lost.time.AfterFuncwrites the latestwb.currentto register261once the duration has passed, honoring any setpoint changes that arrived during the window. The callback queriesEnabled()first and skips the re-apply if the charger was disabled in the meantime.wb.current,wb.phasesandwb.phaseSwitchEndwith async.Mutexsince theAfterFunccallback runs concurrently with the loadpoint goroutine.261,503,504and5001.Background
Based on the approach proposed in the closed #20525, reduced to the timer-based path (the internal-phase-switch / power-register variant was dropped).