feat(battery): forced discharge to grid mode (feed-in arbitrage) - #31995
feat(battery): forced discharge to grid mode (feed-in arbitrage)#31995webalexeu wants to merge 17 commits into
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In applyBatteryMode, the min-SoC reserve enforcement mirrors the existing max-SoC handling but duplicates the pattern; consider extracting a shared helper for these mode-to-hold transitions to keep the charge/discharge guard logic centralized and less error-prone to change.
- batteryMinSocReached returns a hard error when a BatterySocLimiter-capable device lacks Battery capability; if such mixed capability devices are possible, you may want to treat this as ErrNotAvailable (and skip the reserve check) instead of failing the whole battery mode update.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In applyBatteryMode, the min-SoC reserve enforcement mirrors the existing max-SoC handling but duplicates the pattern; consider extracting a shared helper for these mode-to-hold transitions to keep the charge/discharge guard logic centralized and less error-prone to change.
- batteryMinSocReached returns a hard error when a BatterySocLimiter-capable device lacks Battery capability; if such mixed capability devices are possible, you may want to treat this as ErrNotAvailable (and skip the reserve check) instead of failing the whole battery mode update.
## Individual Comments
### Comment 1
<location path="core/site_battery.go" line_range="149-158" />
<code_context>
+// batteryMinSocReached checks if the battery has reached its min soc reserve.
+// Used to stop forced grid discharge before draining the reserve.
+func (site *Site) batteryMinSocReached(dev config.Device[api.Meter]) (bool, error) {
+ meter := dev.Instance()
+
+ batLimiter, ok := api.Cap[api.BatterySocLimiter](meter)
+ if !ok {
+ return false, nil
+ }
+
+ batSoc, ok := api.Cap[api.Battery](meter)
+ if !ok {
+ return false, errors.New("battery with soc limits must have soc")
+ }
+
+ soc, err := batSoc.Soc()
+ if err != nil {
+ return false, err
+ }
+
+ if min, _ := batLimiter.GetSocLimits(); min > 0 && soc <= min {
+ site.log.DEBUG.Printf("battery %s: reserve soc reached (%.0f <= %.0f)", deviceTitleOrName(dev), soc, min)
+ return true, nil
</code_context>
<issue_to_address>
**question:** Min-SoC logic silently ignores a configured limit of 0%, which might be an unintended edge case.
In `batteryMinSocReached`, `min > 0 && soc <= min` means a configured min-SoC of 0% is treated as “no limit”. If 0% should be a valid setting (e.g. “allow full discharge, but still enforce/track a limit”), this condition will never trigger. If the intended meaning is “0 disables the limit”, consider making that explicit (e.g. with a `min == 0` branch or by documenting this behavior where the limit is configured).
</issue_to_address>
### Comment 2
<location path="core/site_battery.go" line_range="183-186" />
<code_context>
// In case max soc is reached, hold mode is applied.
func (site *Site) applyBatteryMode(mode api.BatteryMode) error {
fromToCharge := mode == api.BatteryCharge || mode == api.BatteryUnknown && site.batteryMode == api.BatteryCharge
+ fromToDischarge := mode == api.BatteryDischarge || mode == api.BatteryUnknown && site.batteryMode == api.BatteryDischarge
for _, dev := range site.batteryMeters {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Mutating `mode` inside the device loop affects all subsequent batteries, which might be broader than intended.
In `applyBatteryMode`, `mode` is reused for all devices. If `fromToDischarge` is true and any battery reaches reserve (`batteryMinSocReached`), `mode` is switched to `BatteryHold`, causing all later batteries to be held even if they haven’t reached min-SoC. If Hold is meant to be global, this is correct; otherwise, consider using a per-device `deviceMode` derived from `mode` so you can change it locally without affecting the rest of the loop.
Suggested implementation:
```golang
func (site *Site) applyBatteryMode(mode api.BatteryMode) error {
fromToCharge := mode == api.BatteryCharge || mode == api.BatteryUnknown && site.batteryMode == api.BatteryCharge
fromToDischarge := mode == api.BatteryDischarge || mode == api.BatteryUnknown && site.batteryMode == api.BatteryDischarge
for _, dev := range site.batteryMeters {
deviceMode := mode
meter := dev.Instance()
}
}
// validate min soc reserve when discharging to grid
if fromToDischarge && deviceMode != api.BatteryHold {
ok, err := site.batteryMinSocReached(dev)
```
To fully implement per-device mode handling and avoid unintended global mode mutation:
1. Inside the `for _, dev := range site.batteryMeters` loop, any place where `mode` is:
- mutated (e.g. `mode = api.BatteryHold`), or
- read to decide behavior for that specific battery (e.g. `if mode == api.BatteryDischarge { ... }`, `switch mode { ... }`),
should be switched to use `deviceMode` instead, e.g. `deviceMode = api.BatteryHold`, `if deviceMode == ...`, `switch deviceMode { ... }`.
2. If `site.batteryMode` is intended to reflect the *global* mode, ensure it is updated explicitly outside or after the per-device loop as needed, rather than indirectly via mutations inside the loop.
You’ll need to apply these replacements to the rest of the `applyBatteryMode` function where the snippet did not show all uses of `mode`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Now you're pushing me ;) |
20d1bc6 to
c07fedb
Compare
|
I really like the PR. The optimizer already has
I've added #32086 to control this. |
|
@copilot resolve the merge conflicts in this pull request |
032c6ca to
8464168
Compare
I've resolved the conflicts. I'm on holidays for the next two weeks so I will probably be less responsive |
Brings in ~197 upstream commits including the battery grid-discharge groundwork (batteryGridDischarge setting evcc-io#32086, discharge advisory action evcc-io#32087 - the actual forced-discharge battery mode, PR evcc-io#31995, is still open upstream), the solar-adjusted forecast setting, HEMS curtailment feed into the optimizer's grid export limit, and the meter.go battery capability config migration to static-or-plugin values. Conflict highlights: - currentSlotSuggestion(): adopted upstream's lazy-actionable design (Actionable now computed on read via batterySuggestion()/ loadpointSuggestion() instead of baked in at computation time) while keeping our holdChargeSuggestions population for the battery-mode automation (Paket A) and the new discharge case for grid export. - solar forecast scale: combined our 28-day robust median (solarScaleMedian(), used as the optimizer's base scale) with upstream's newer intra-day blend refinement (blendScale using the last completed slot's measured/forecast ratio) - additive, not competing. - meter.go: migrated our MaxChargePowerLimit/ChargeSetpoint push-config fields onto upstream's new Ctx-based capability types (batteryCapacityCtx/batterySocLimitsCtx/batteryPowerLimitsCtx), which now accept either a static value or a plugin config. Verified: go build/vet/test (core, meter, server - excluding two pre-existing environment-dependent failures confirmed present on pristine upstream/master too: meter template tests needing local device simulators, and the eebus TestShipPairing network handshake test), vue-tsc, lint:i18n, vitest (170/171, the one failure is an unrelated pre-existing 12h time-format locale test). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AXmAz8YRFAKgg8ZjA2daCh
|
@webalexeu please also add discharge option to |
8464168 to
63426d7
Compare
Good catch. PR updated accordingly |
5bbee88 to
58e30c2
Compare
58e30c2 to
3637e92
Compare
|
@andig @premultiply Can you please review it ? |
|
@webalexeu can we keep the devices separate? Would also help to not force-push, it makes incremental review impossible. |
3637e92 to
ab401bf
Compare
Symmetric counterpart to batteryGridChargeLimit: discharge the home battery to the grid when the feed-in rate is at or above a configurable batteryGridDischargeLimit, after house/EV priority and down to the battery's min-soc reserve. - new api.BatteryDischarge battery mode - batteryGridDischargeLimit setting (site api, http, mqtt, persistence) - requiredBatteryMode discharges when feed-in >= limit; grid charge and the EV discharge-control hold take precedence - min-soc reserve guard in applyBatteryMode Refs evcc-io#31971
An unmatched case means the device does not implement the requested setting. Callers that probe optional capabilities (battery mode) can then skip the device instead of aborting the whole update loop.
- hold grid discharge while HEMS curtails production, mirroring the existing dimmed/charge gate: force-exporting while the operator curtails production is the wrong direction - restore the hard error for a soc-limited battery without soc reading; turning it into ErrNotAvailable silently disabled max-soc validation for grid charge - log the charge/discharge conflict inside the charge case and only on entry, so it no longer fires under external control or repeats every cycle - rename min/max soc locals so they stop shadowing the builtins
…tion action The optimizer's discharge advisory now has a matching api.BatteryMode, so the separate actionDischarge constant collapses into api.BatteryDischarge.String() and the actionable check works like the other three battery actions.
BATTERY_MODE gains DISCHARGE (regenerated state schema and mcp/openapi.json), BatteryModeResult allows 5, and POST/DELETE /batterygriddischargelimit are documented like their grid charge twins.
|
@webalexeu sorry for the confusion — that remark was about device support, not a request. I've pushed four commits to this branch that work through my review above, so you don't have to:
Left open, and I'd like your input:
|
Same entering/holding/leaving check was spelled out four times across updateBatteryMode and applyBatteryMode.
|
Two follow-ups on top of those four: |
…arge control dischargeControlActive is opt-in and off by default, so relying on it alone left grid-sell discharge free to run while an EV fast-charges when the toggle isn't set - contradicting the PR description's "after house/EV priority" claim. Add evFastChargingActive as an unconditional check scoped to the discharge branch only; the existing charge/hold toggle behavior is untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Pushed one more commit on top: Finding 9: Finding 8: agreed it's a real config trap. Since there's no UI for |
…-discharge # Conflicts: # server/http.go # server/mcp/openapi.md
|
@webalexeu @naltatis we've seen that discharging to grid is regulated in some geographies (Germany...). It seems that:
|
|
Commit coming up. Except for that- are we good to merge? |
The limit forced discharge to grid on its own, while the experimental grid discharge setting only reached the optimizer. Both now share one opt-in.
Ok for me |
… toggle The limit was API-only. It now gets a card next to grid charging, shown only while the experimental grid discharge setting is on, where the limit applies.
|
@naltatis @webalexeu tried adding the UI setting too to make sure it contains the gating logic. Could you kindly check? |
The control reused the grid charge precondition, so it needed a dynamic grid price instead of a dynamic feed-in tariff, and rendered an empty card without one. The e2e config gains a feed-in tariff so the test sees a real control.
webalexeu
left a comment
There was a problem hiding this comment.
Checked the gating logic per your ask. Found one gap: gridDischargeVisible hides the discharge-limit card whenever the experimental toggle is off, even if a limit is already set — the opposite of what the comment right above it says ("a limit that is already set stays reachable so it can be removed"). The new e2e test only covers toggle-off+no-limit and toggle-on, so it didn't catch this. Suggested fix inline. Everything else — the backend gate in batteryGridDischargeActive, the isLoadpoint/apply-all guard, and the toggle switch in BatteryConfigCard.vue — looks correct.
| gridDischargeVisible(): boolean { | ||
| return ( | ||
| !!this.state.batteryGridDischarge && | ||
| (this.gridDischargePossible || this.gridDischargeLimit !== null) | ||
| ); | ||
| }, |
There was a problem hiding this comment.
This hides the card whenever batteryGridDischarge is off, even when a limit is already set — contradicting the comment above it. Compare gridChargeVisible just above (no such gate, correctly keeps a stale limit reachable/removable).
| gridDischargeVisible(): boolean { | |
| return ( | |
| !!this.state.batteryGridDischarge && | |
| (this.gridDischargePossible || this.gridDischargeLimit !== null) | |
| ); | |
| }, | |
| gridDischargeVisible(): boolean { | |
| return ( | |
| this.gridDischargeLimit !== null || | |
| (!!this.state.batteryGridDischarge && this.gridDischargePossible) | |
| ); | |
| }, |
The experimental batteryGridDischarge setting only gated whether the limit acts (batteryGridDischargeActive), not whether it exists. A limit set with the setting on stayed stored, published and restored on startup after the setting was turned off - inert, but unreachable from the UI and silently live again on the next restart. The limit could also be set via REST/MQTT with the setting off, creating the same orphan. Refuse a limit while the setting is off, drop it when the setting is turned off, and skip restoring a stored limit that has no opt-in behind it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up on evcc-io#31995. Adds forced discharge to grid (feed-in arbitrage) support to marstek-venus-e.yaml (Venus E/Gen2/Gen3/Venus C) and marstek-venus-a.yaml (Venus A/D). The batterymode sequence (cases 1-5: normal/hold/charge/holdcharge/ discharge) was identical between both templates, so it's extracted into a shared util/templates/includes/marstek-batterymode.tpl fragment, following the same include pattern already used for battery-params/modbus. maxdischargepower has a representative default (800W) on Venus E, which covers four hardware variants under one template, but is required (no default) on Venus A/D, whose rated discharge power differs per model with no verified spec to default to - mirroring how maxchargepower is already handled asymmetrically between these two templates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-discharge # Conflicts: # assets/js/settings.ts # core/site_battery.go # core/site_battery_test.go # server/http.go
|
@andig tested locally and seems to be good on the UI side |
|
I'll give this a closer look and will provide feedback soon. |
Refs #31971
Implements the forced discharge battery mode that @andig flagged as the missing prerequisite ("we'd require a forced discharge battery mode which is not available"). It adds that mode plus the symmetric feed-in counterpart to the existing
batteryGridChargeLimit, so grid arbitrage works in both directions.Opt-in and region-dependent by construction: inactive unless
batteryGridDischargeLimitis set. When set, the battery discharges to grid while the feed-in rate is at or above the limit — after house/EV priority, and only down to the battery's min-soc reserve.api.BatteryDischargemode, appended to the enum so existing persisted mode values are unchangedbatteryGridDischargeLimitsetting mirroringbatteryGridChargeLimit(site API, HTTP, MQTT, persistence)requiredBatteryMode: discharge when the feed-in rate ≥ limit; grid-charge and the EV discharge-control hold both take precedenceapplyBatteryMode(falls back to hold before draining the reserve)On device support (@andig's "verify how common devices would support this", and @Thomvh's note that NL batteries commonly have a "sell" mode): the new mode reuses the existing battery-mode plumbing. A custom meter's
batteryModesetter receives it as an int/string (5/discharge) and maps it to the device's sell command; SoC-limit-only batteries returnErrNotAvailableand are unaffected.TODO