Optimizer: automatic mode - #32881
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In setLoadpointsLimit, when there are no active loadpoints lastErr can remain nil while applied is false, causing a silent success even though nothing accepted the limit; consider explicitly returning a sentinel error in that case to match the HTTP handler’s "fails only if none accepted it" semantics.
- Suggestion staleness is checked against time.Now() in site.suggestion and lp.clock in Loadpoint.gate; to avoid subtle timebase inconsistencies (especially with mocked clocks), consider using a consistent clock source for both paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In setLoadpointsLimit, when there are no active loadpoints lastErr can remain nil while applied is false, causing a silent success even though nothing accepted the limit; consider explicitly returning a sentinel error in that case to match the HTTP handler’s "fails only if none accepted it" semantics.
- Suggestion staleness is checked against time.Now() in site.suggestion and lp.clock in Loadpoint.gate; to avoid subtle timebase inconsistencies (especially with mocked clocks), consider using a consistent clock source for both paths.
## Individual Comments
### Comment 1
<location path="server/http_site_handler.go" line_range="238-247" />
<code_context>
}
+// setLoadpointsLimit applies a limit to all loadpoints that accept it
+func setLoadpointsLimit(site site.API, setLimit func(loadpoint.API, *float64) error, val *float64) error {
+ var applied bool
+ var lastErr error
+
+ for _, lp := range site.ActiveLoadpoints() {
+ if err := setLimit(lp, val); err != nil {
+ lastErr = err
+ } else {
+ applied = true
+ }
+ }
+
+ if applied {
+ return nil
+ }
+
+ return lastErr
+}
+
</code_context>
<issue_to_address>
**question:** `setLoadpointsLimit` silently succeeds when there are no active loadpoints and no error occurred.
When `ActiveLoadpoints()` returns no loadpoints, `applied` stays false and `lastErr` is nil, so the function returns `nil` even though no limit was applied. This differs from the intended "request fails only if none accepted it" semantics in `updateSmartCostLimit`, because here "none" is due to having no candidates. If that distinction is important, consider returning a specific error (e.g. `ErrNoLoadpoints`) when there are no active loadpoints so callers can surface that nothing was applied.
</issue_to_address>
### Comment 2
<location path="core/optimizer_automatic_test.go" line_range="22" />
<code_context>
+)
+
+// enableAutomatic puts the optimizer in control for the duration of the test
+func enableAutomatic(t *testing.T) {
+ t.Helper()
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a focused test for errorStatus and HTTP 409 mapping when optimizer automatic mode blocks settings
`errorStatus` now maps `ErrOptimizerAutomatic` to HTTP 409, which is important client-facing behaviour when automatic mode is active. Existing tests cover `ErrOptimizerAutomatic` being returned, but not that `errorStatus` (and the HTTP handlers using it) emit 409 in those cases and 400 otherwise. Please add a focused unit test for `errorStatus`, or a handler test that triggers a blocked setting and asserts `StatusConflict`, to lock in this contract and prevent regressions.
Suggested implementation:
```golang
import (
"testing"
"time"
"net/http"
evbus "github.com/asaskevich/EventBus"
"github.com/benbjohnson/clock"
"github.com/evcc-io/evcc/api"
"github.com/evcc-io/evcc/core/keys"
coresettings "github.com/evcc-io/evcc/core/settings"
"github.com/evcc-io/evcc/core/types"
"github.com/evcc-io/evcc/server/db/settings"
"github.com/stretchr/testify/assert"
)
// TestErrorStatusOptimizerAutomatic verifies that ErrOptimizerAutomatic is mapped to HTTP 409
// while other errors (e.g. validation errors) still map to HTTP 400.
func TestErrorStatusOptimizerAutomatic(t *testing.T) {
t.Run("automatic mode blocks settings -> 409", func(t *testing.T) {
status := errorStatus(coresettings.ErrOptimizerAutomatic)
assert.Equal(t, http.StatusConflict, status)
})
t.Run("validation error -> 400", func(t *testing.T) {
status := errorStatus(settings.ErrValidation)
assert.Equal(t, http.StatusBadRequest, status)
})
}
```
- Ensure that `errorStatus` is in the same package as `core/optimizer_automatic_test.go`. If it lives in a different package (e.g. a `server` package), this test file may need to use that package name in its imports and call `server.ErrorStatus(...)` or similar instead of a bare `errorStatus(...)`.
- If `settings.ErrValidation` is not the canonical "validation/400" error in your codebase, replace it with whatever error type currently maps to `http.StatusBadRequest` in `errorStatus`.
- If `github.com/evcc-io/evcc/server/db/settings` and `github.com/stretchr/testify/assert` are already imported elsewhere in this file, deduplicate the imports so they appear only once in the import block.
- If your existing tests use table-driven style, you can refactor `TestErrorStatusOptimizerAutomatic` into a table-driven test while keeping the same assertions for 409 and 400 to match local conventions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
UI update: automatic mode is now visible throughout the app.
|
|
Nice iteration. Most critical part from my perspective is to maintain precise solar surplus charging behavior for optimizer controlled loadpoints in situations where no grid import is intended by the optimizer result. People really hate pulling from or feeding into the grid if it's avoidable (see battery boost discussions 😅). If this is solved (I don't know the elegant solution) we could give this out for testing. |
|
I've added screenshots to the description. |
@naltatis this is solved |
Let the optimizer control charging start/stop and the home battery instead of only advising. Price limits, discharge control and planner slots give way to its decision, off and fast charging stay with the user.
Suggestion-based gating so a stalled optimizer releases the planner, plan overrun backstop, welcome charge and climate control in the stop path, limits read as unset while the optimizer replaces them, battery hold for loads it cannot model, and config errors that no longer get swallowed.
Drop the slot-duration gate and trigger a run from loopLoadpoints so suggestions are refreshed each time the loadpoints have been cycled through. minAge and optimizerUpdated become unused as all remaining callers forced a run anyway. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Only automatic mode needs a fresh result per loadpoint cycle for the loadpoint gate; advisory suggestions stay at one run per slot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Structured what-changes/what-stays list in the optimizer settings, automatic mode strip on the optimize page, notes with the new auto icon on battery and grid charging settings, per-loadpoint status icon while the optimizer gates charging, and hidden price limit badge on controlled loadpoints. Success surface colors derive from the brand green via new global tokens.
…point and pv surplus
8ab2b84 to
1a9329b
Compare
|
Tested it today, one quick thing I realized is that the charger get's enabled/disabled by optimizer decisions quite often. Do we need to have a hardware protection respecting the PV timer thresholds? As the optimizer results may vary on execution (every cycle) it sounds like an important requirement for EV chargers at least? |
|
Thanks for giving this a try! Did you see why this happens? I‘ve seen the optimizer chose a different battery first which is not what you‘d like to do normally. We could also try to fix this via the timers or just run the optimizer less often. Maybe every 5min is sufficient? |
|
I think it was several factors, with the decisive one being the fluctuating PV yield. I'm going to try to confirm this with a trace on the next test run. Right now I only have the PV yield logs, which point in that direction, but not the optimizer requests/responses. |
|
Evaluating the available traces in more detail I see the "other battery first" as well. Your questioning is difficult to answer. I'd go for the timers and probably to address kind of solver instability (costs are within 1ct.)? To optimize less often would not help much. Thinking loud: what about thresholds and using c_priority to model the preferences, i.e. as long as battery soc is below x set c_prio to 1, else 0 and probably giving priority at other loadpoints? |
This comment was marked as resolved.
This comment was marked as resolved.
|
As usual a log would help. I can run Claude myself. |
|
But you have not my setup and observations. I think claude helps a lot to analyse log files. I just want to help. |
Absolutely. And that requires the logs (still missing...) ;) |
Adds an automatic mode to the optimizer. So far the optimizer only calculated and displayed suggestions, with automatic mode enabled it acts on them.
pvandminpvmode the optimizer decides when charging starts and stops. It only gates charging, current and phase selection stay with the loadpoint.offand fast charging remain the user's decision. The optimizer plans around them and never overrides them.409 Conflictwhile automatic mode is active: per loadpoint smart cost limit and feed-in priority limit, battery grid charge limit and battery discharge control. Stored values are kept and take effect again once automatic mode is switched off.UI
Screenshots
on/off in optimizer debug (short explain)

on/off in optimizer debug mobile (short explain)

config modal (full explain)

battery settings

loadpoint in auto mode (only in (min)pv mode)

loadpoint settings

TODO
🤖 Generated with Claude Code