Powerwall: add dynamic battery parameters - #32742
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
newPowerWall,powerLimitercan remainnil(e.g. whenMaxApparentPoweris 0) and is still passed toimplement.BatteryPowerLimiter, which may panic or behave unexpectedly; consider guardingimplement.Maycalls so they are only invoked when the corresponding limiter function is non-nil. - Similarly in
NewPowerWallFleetFromConfig,controllerfromLimitControllermight benileven whenerr == nil; it would be safer to skipimplement.May(m, implement.BatteryController(controller))when no controller is configured instead of relying on downstream handling of a nil function.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `newPowerWall`, `powerLimiter` can remain `nil` (e.g. when `MaxApparentPower` is 0) and is still passed to `implement.BatteryPowerLimiter`, which may panic or behave unexpectedly; consider guarding `implement.May` calls so they are only invoked when the corresponding limiter function is non-nil.
- Similarly in `NewPowerWallFleetFromConfig`, `controller` from `LimitController` might be `nil` even when `err == nil`; it would be safer to skip `implement.May(m, implement.BatteryController(controller))` when no controller is configured instead of relying on downstream handling of a nil function.
## Individual Comments
### Comment 1
<location path="meter/powerwall.go" line_range="121-123" />
<code_context>
- implement.Has(m, implement.BatteryCapacity(func() float64 {
- return res.NominalFullPackEnergy / 1e3
- }))
+ if minG == nil {
+ opG := util.Cached(client.GetOperation, cc.Cache)
+ minG = func() float64 {
+ op, err := opG()
+ if err != nil {
</code_context>
<issue_to_address>
**issue (bug_risk):** Backup reserve fallback silently treats errors as 0% SoC limit, which can be risky.
In the `minG` fallback, any `opG()` error results in returning `0`, effectively removing the lower SoC bound and allowing deeper discharge when the operation endpoint is unavailable. Please either surface the error (so setup fails explicitly) or choose a safer default (e.g., last known or a conservative reserve) and log the failure for visibility.
</issue_to_address>
### Comment 2
<location path="meter/powerwall.go" line_range="111" />
<code_context>
- implement.Has(m, implement.Battery(m.batterySoc))
- implement.May(m, implement.BatterySocLimiter(cc.batterySocLimits.Decorator()))
- implement.May(m, implement.BatteryPowerLimiter(cc.batteryPowerLimits.Decorator()))
+ capacity, err := cc.batteryCapacityCtx.Decorator(ctx)
+ if err != nil {
+ return nil, err
</code_context>
<issue_to_address>
**issue (complexity):** Consider moving the battery fallback and nil-handling logic into config-type methods or a helper so `newPowerWall` stays a short, declarative wiring function.
You can keep the new ctx‑aware behavior while reducing complexity by pushing the fallback logic back into the config types and/or a small helper, so `newPowerWall` stays mostly declarative.
### 1. Move policy into config types
`newPowerWall` currently decides:
- how to derive `minG`/`maxG` (including `GetOperation` calls and defaults),
- whether to call `GetSystemStatus`,
- how to fall back for capacity and power limits.
All of that can live inside the `battery*Ctx` types, keeping `newPowerWall` focused on wiring:
```go
func newPowerWall(ctx context.Context, log *util.Logger, cc powerWallConfig) (*PowerWall, error) {
// ... client setup omitted ...
m := &PowerWall{
log: log,
client: client,
usage: strings.ToLower(cc.Usage),
meterG: util.Cached(client.GetMetersAggregates, cc.Cache),
}
if m.usage == "load" || m.usage == "solar" {
implement.Has(m, implement.MeterEnergy(m.totalEnergy))
}
if m.usage == "battery" {
socLimiter, err := cc.batterySocLimitsCtx.Decorator(ctx, client, cc.Cache)
if err != nil {
return nil, err
}
capacity, powerLimiter, err := cc.batteryPowerConfig.Decorators(ctx, client, cc.Cache)
if err != nil {
return nil, err
}
implement.Has(m, implement.Battery(m.batterySoc))
implement.May(m, implement.BatterySocLimiter(socLimiter))
implement.May(m, implement.BatteryPowerLimiter(powerLimiter))
implement.Has(m, implement.BatteryCapacity(capacity))
}
return m, nil
}
```
Then encapsulate all current `nil`/fallback logic in the config types:
```go
// example only – adapt to your actual types
func (c batterySocLimitsCtx) Decorator(ctx context.Context, client *powerwall.Client, cache time.Duration) (func() (float64, float64), error) {
minG, maxG, err := c.getters(ctx)
if err != nil {
return nil, err
}
if minG == nil {
opG := util.Cached(client.GetOperation, cache)
minG = func() float64 {
op, err := opG()
if err != nil {
return 0
}
return op.BackupReservePercent
}
}
if maxG == nil {
maxG = func() float64 { return 100 }
}
return func() (float64, float64) {
return minG(), maxG()
}, nil
}
func (c batteryPowerLimitsCtx) Decorators(ctx context.Context, client *powerwall.Client, cache time.Duration) (
capacity func() float64,
powerLimiter func() (float64, float64),
err error,
) {
capacity, err = c.capacityDecorator(ctx)
if err != nil {
return nil, nil, err
}
powerLimiter, err = c.powerDecorator(ctx)
if err != nil {
return nil, nil, err
}
if capacity != nil && powerLimiter != nil {
return capacity, powerLimiter, nil
}
res, err := client.GetSystemStatus()
if err != nil {
return nil, nil, err
}
if capacity == nil {
capacity = func() float64 { return res.NominalFullPackEnergy / 1e3 }
}
if powerLimiter == nil && res.MaxApparentPower > 0 {
powerLimiter = func() (float64, float64) {
return res.MaxApparentPower, res.MaxApparentPower
}
}
return capacity, powerLimiter, nil
}
```
This keeps all the “which source wins when?” policy in a dedicated, testable place and restores `newPowerWall` to a short wiring function.
### 2. Optional: extract a small helper instead of inlining
If you prefer not to change config types right now, at least extract the battery block into a helper so `newPowerWall` remains readable:
```go
func configureBattery(
ctx context.Context,
m *PowerWall,
cc powerWallConfig,
) error {
capacity, err := cc.batteryCapacityCtx.Decorator(ctx)
if err != nil {
return err
}
minG, maxG, err := cc.batterySocLimitsCtx.getters(ctx)
if err != nil {
return err
}
// ... current minG/maxG/capacity/powerLimiter logic moved here ...
implement.Has(m, implement.Battery(m.batterySoc))
implement.May(m, implement.BatterySocLimiter(socLimiter))
implement.May(m, implement.BatteryPowerLimiter(powerLimiter))
implement.Has(m, implement.BatteryCapacity(capacity))
return nil
}
```
Then `newPowerWall` only calls:
```go
if m.usage == "battery" {
if err := configureBattery(ctx, m, cc); err != nil {
return nil, err
}
}
```
Both approaches keep the new functionality but reduce nesting and interwoven `nil` checks in `newPowerWall`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| advanced: true | ||
| - name: maxchargepower | ||
| - name: maxdischargepower | ||
| - preset: battery-params |
There was a problem hiding this comment.
Soc/powers must be removed, i.e. deprecated when using plugins. Battery-Params re-adds all of them, so can‘t be used.
| RefreshToken_ string `mapstructure:"refreshToken"` // TODO deprecated | ||
| batterySocLimits `mapstructure:",squash"` | ||
| batteryPowerLimits `mapstructure:",squash"` | ||
| batteryCapacityCtx `mapstructure:",squash"` |
There was a problem hiding this comment.
These are hard-wired now, so no point having them at all being part of the config once manual settings (which don‘t make much sense) are removed.
|
@mfuchs1984 @GrimmiMeloni simplified and streamlined. |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The description mentions still honoring configured maxsoc/maxchargepower/maxdischargepower, but the new implementation only logs a deprecation warning and ignores these values for the local meter; consider either actually using them as a fallback when the API is unavailable or removing them entirely to avoid confusing behavior.
- Battery power limiting for the local meter is now only enabled when status.MaxApparentPower > 0; if this field is missing or zero, users lose the ability to constrain charge/discharge power, so you may want a defined fallback (e.g., sensible defaults or deprecated config fields) rather than disabling the limiter completely.
- The error logging inside the dynamic BatteryCapacity/BatterySocLimiter/BatteryPowerLimiter callbacks will be triggered on every polling failure, which could be noisy in intermittent error scenarios; consider downgrading log level or rate-limiting these messages.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The description mentions still honoring configured maxsoc/maxchargepower/maxdischargepower, but the new implementation only logs a deprecation warning and ignores these values for the local meter; consider either actually using them as a fallback when the API is unavailable or removing them entirely to avoid confusing behavior.
- Battery power limiting for the local meter is now only enabled when status.MaxApparentPower > 0; if this field is missing or zero, users lose the ability to constrain charge/discharge power, so you may want a defined fallback (e.g., sensible defaults or deprecated config fields) rather than disabling the limiter completely.
- The error logging inside the dynamic BatteryCapacity/BatterySocLimiter/BatteryPowerLimiter callbacks will be triggered on every polling failure, which could be noisy in intermittent error scenarios; consider downgrading log level or rate-limiting these messages.
## Individual Comments
### Comment 1
<location path="meter/powerwall.go" line_range="115" />
<code_context>
}
if m.usage == "battery" {
- implement.Has(m, implement.Battery(m.batterySoc))
- implement.May(m, implement.BatterySocLimiter(cc.batterySocLimits.Decorator()))
</code_context>
<issue_to_address>
**issue (complexity):** Consider factoring the repeated cached status/operation calls and error logging into small helper functions to make the battery-usage block’s control flow more linear and readable.
You can simplify the new wiring by centralizing the cached calls + logging into small helpers, and reusing them instead of repeating `statusG()` / `opG()` and error handling in each closure.
For example, inside the `if m.usage == "battery"` block:
```go
if m.usage == "battery" {
statusG := util.Cached(client.GetSystemStatus, cc.Cache)
opG := util.Cached(client.GetOperation, cc.Cache)
// helper wraps cached status + logging
getStatus := func(context string) (powerwall.SystemStatus, bool) {
res, err := statusG()
if err != nil {
log.ERROR.Println(context+":", err)
return powerwall.SystemStatus{}, false
}
return res, true
}
// helper wraps cached operation + logging
getOperation := func() (powerwall.Operation, bool) {
op, err := opG()
if err != nil {
log.ERROR.Println("battery soc limits:", err)
return powerwall.Operation{}, false
}
return op, true
}
// validate connectivity and gate power limiter capability
status, ok := getStatus("connectivity")
if !ok {
return nil, fmt.Errorf("powerwall connectivity failed")
}
implement.Has(m, implement.Battery(m.batterySoc))
implement.Has(m, implement.BatteryCapacity(func() float64 {
res, ok := getStatus("battery capacity")
if !ok {
return 0
}
return res.NominalFullPackEnergy / 1e3
}))
implement.Has(m, implement.BatterySocLimiter(func() (float64, float64) {
op, ok := getOperation()
if !ok {
return 0, 100
}
return op.BackupReservePercent, 100
}))
if status.MaxApparentPower > 0 {
implement.Has(m, implement.BatteryPowerLimiter(func() (float64, float64) {
res, ok := getStatus("battery power limits")
if !ok {
return 0, 0
}
return res.MaxApparentPower, res.MaxApparentPower
}))
}
}
```
This keeps:
- dynamic values from the device (still via `util.Cached`),
- connectivity gating for the power limiter based on the first `status`,
- the same fallbacks/limits,
but removes the repeated `statusG()` / `opG()` calls with inline logging and error handling, making the control flow more linear and easier to follow.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Awesome, you're on a streak those days, will give it a try later today. |
|
@andig this warning never triggers, the parameters are not passed to the go code, they stay nil, is this intentional? if cc.MaxSoc_ != nil || cc.MaxChargePower_ != nil || cc.MaxDischargePower_ != nil {
log.WARN.Println("maxsoc, maxchargepower and maxdischargepower are deprecated, values are read from the device")
} |
|
Apart from that, it works. |
|
Nice to see that the minSoc now seamlessly adapts when changing it in the Tesla app, no more manual synchronization needed :) |
|
Does this respect the 80-100% minSoc deadzone? |
|
Is this relevant when reading the backup reserve? |
No. |
|
Then, the change should IMHO be fine |
This is an attempt to use the dynamic battery parameters introduced by #31668 for the tesla powerwall to get
minSoc,maxChargePowerandmaxDischargePower.maxSocis always 100%, afaik, it cannot be changed.capacityis also dynamic now, since it can change during runtime of evcc.Currently, it's still possible to define the parameters in the configuration, then, evcc won't use the API and instead, use the configured values. Not sure if user configuration could be completely removed to simplify configuration for the use.
Tested with my Powerwall 2.