Zaptec Go2: use ThreeToOnePhaseSwitchCurrent for switching#29587
Zaptec Go2: use ThreeToOnePhaseSwitchCurrent for switching#29587tbaehler wants to merge 1 commit into
Conversation
…set power to 0 when disconnected
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Using
Status()to triggerMaxCurrentMillis(0)introduces a side effect in what is typically a read-only operation; consider moving this state change into a more explicit control path or clearly documenting thatStatus()mutates charger state. - The
lastStatusfield defaults to zero, which may equalOpModeDisconnectedand suppress the first transition handling; consider initializing it to a sentinel value (e.g., via a constructor) that cannot collide with valid operation modes. - The hard-coded
phaseSwitchCurrent = 32for Go2 phase switching is a magic number; consider extracting it into a well-named constant to make the behavior and reasoning clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `Status()` to trigger `MaxCurrentMillis(0)` introduces a side effect in what is typically a read-only operation; consider moving this state change into a more explicit control path or clearly documenting that `Status()` mutates charger state.
- The `lastStatus` field defaults to zero, which may equal `OpModeDisconnected` and suppress the first transition handling; consider initializing it to a sentinel value (e.g., via a constructor) that cannot collide with valid operation modes.
- The hard-coded `phaseSwitchCurrent = 32` for Go2 phase switching is a magic number; consider extracting it into a well-named constant to make the behavior and reasoning clearer.
## Individual Comments
### Comment 1
<location path="charger/zaptec.go" line_range="384-389" />
<code_context>
- AvailableCurrentPhase2: &c.maxCurrent,
- AvailableCurrentPhase3: &c.maxCurrent,
- }
+ var phaseSwitchCurrent float64
+
+ if phases == 1 {
+ phaseSwitchCurrent = 32 // threshold which Go2 switches to single-phase
+ } else {
+ phaseSwitchCurrent = 0
}
</code_context>
<issue_to_address>
**suggestion:** The hardcoded `32` threshold for phase switching should be clarified or centralized.
Since `phaseSwitchCurrent = 32` is a protocol-/product-specific threshold, please extract it into a named constant (and/or reference its source in a comment) so it’s clear what this value represents and easier to update in the future.
Suggested implementation:
```golang
const zaptecGo2PhaseSwitchCurrent = 32.0 // threshold (in A) at which Zaptec Go 2 switches to single-phase according to product/protocol specs
func init() {
}
```
```golang
if phases == 1 {
phaseSwitchCurrent = zaptecGo2PhaseSwitchCurrent
} else {
phaseSwitchCurrent = 0
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| var phaseSwitchCurrent float64 | ||
|
|
||
| if phases == 1 { | ||
| phaseSwitchCurrent = 32 // threshold which Go2 switches to single-phase | ||
| } else { | ||
| phaseSwitchCurrent = 0 |
There was a problem hiding this comment.
suggestion: The hardcoded 32 threshold for phase switching should be clarified or centralized.
Since phaseSwitchCurrent = 32 is a protocol-/product-specific threshold, please extract it into a named constant (and/or reference its source in a comment) so it’s clear what this value represents and easier to update in the future.
Suggested implementation:
const zaptecGo2PhaseSwitchCurrent = 32.0 // threshold (in A) at which Zaptec Go 2 switches to single-phase according to product/protocol specs
func init() {
} if phases == 1 {
phaseSwitchCurrent = zaptecGo2PhaseSwitchCurrent
} else {
phaseSwitchCurrent = 0
}There was a problem hiding this comment.
It is not even clear whether this special Go 2 logic is needed. I would wait for the Zaptec response on the bug report. Moreover, there might be an alternative API call if the official one is not working to set the phases directly on the charger instead of manipulating the installation.
There was a problem hiding this comment.
You’re absolutely right to be cautious here.
Given that:
- We’re not yet sure this Go 2–specific behavior is required
- The previous API path (setting phases via
MaxChargePhases) is still valid for Go 1 Pro - There may be a better, dedicated API to switch phases without touching the installation
it makes sense to keep this change as minimal and “rollback-friendly” as possible until we have confirmation from Zaptec.
My suggestion for now:
-
Keep the current logic but clearly mark it as provisional:
- Add a comment explaining that this is a workaround pending Zaptec’s response and may be replaced by a proper phase-switch API.
- Optionally guard it with a small helper so it’s easy to swap out later.
-
Still avoid magic numbers even for experimental code:
- Putting
32into a named constant (or at least documenting it in a comment) will make it obvious to future readers that this is a protocol-/behavior-derived value and not arbitrary.
- Putting
For example:
// zaptecGo2PhaseSwitchCurrent is the tentative threshold in A at which
// Zaptec Go 2 switches from three-phase to single-phase.
// This is based on observed behavior and may change once Zaptec
// confirms the correct API and semantics.
const zaptecGo2PhaseSwitchCurrent = 32.0
func (c *Zaptec) switchPhases(phases int) error {
if c.version == zaptec.ZaptecGo1_Pro {
// existing behavior ...
}
var phaseSwitchCurrent float64
if phases == 1 {
phaseSwitchCurrent = zaptecGo2PhaseSwitchCurrent
} else {
phaseSwitchCurrent = 0
}
// use ThreeToOnePhaseSwitchCurrent etc...
}If you prefer, we could also consider temporarily disabling the Go 2–specific branch entirely (keeping only the Go 1 Pro path) until Zaptec responds, and track the experiment in an issue. That would avoid shipping potentially incorrect behavior.
Let me know which direction you’d like to take (keep with clear “experimental” labeling vs. disable until clarified), and I can help refine the structure/comments accordingly.
|
It is not even clear whether this special Go 2 logic is needed. I would wait for the Zaptec response on the bug report. Moreover, there might be an alternative API call if the official one is not working to set the phases directly on the charger instead of manipulating the installation. It would be better to have a common approach for all Zaptec models instead of handling them differently. |
|
@tbaehler pls reference issue this fixes in the pr description (fix xyz) |
|
From what I understand, fix was not agreed on and/or not tested? Let me know if reopening would help. |
|
@andig after further consultations with Zaptec it is currently not very likely they will enable phase switching at charger level for the Go 2. The assumption is that over 90% of Go 2 are in a single-wallbox installation and they prefer the documented way via threeToOnePhaseSwitchCurrent as the Zaptec Sense works in parallel as a Load Balancer to make the installation §14a compliant and the Load Balancer relies on the installation settings. Hence, this approach here seems the best we can achieve for the moment, so please reopen. |
|
Let ne know when good to merge. |
fix 29587
Use new ThreeToOnePhaseSwitchCurrent switching method for go2 and set power to 0