Loadpoint: limit boost power to battery limits#28178
Conversation
|
More demand for this feature in #28336 |
|
This is really 2 PRs in one: limit boosting to max discharge power and publishing that value. I'd appreciate only doing the first part and adding the latter part when we know if we want to do the same for max charge power. I'm also not convinced that is a very interesting value to publish. |
|
Thanks for the feedback. Needs some work for edge cases like batteries with low discharge power etc. anyhow. Will do some testing and work in it when I'm back from vacations. |
|
@andig @premultiply @naltatis what is the expectation when battery boost is enabled, should charging stop when |
|
I wrote a characterization test for the current behavior on the master. It looks like in case there is not enough pv and battery power, when enabling the boost, it will start charging and then stop it after the disable timer elapses. Was able to confirm this behavior as well on my production system by setting min current to 16 A and fixing the phases to 3. With this PR, we have the change to
Both seems more intuitive than the current behavior. |
This reverts commit cca68f7.
Confirmed by #30456 (comment) |
Done with the last (simple) commit. Could also be done independent from this PR. |
|
During local testing, it worked like a charm Concerning
-> This PR won't be effective with an inverter at the AC limit (PV + Battery Discharge == maxAcPower) and boost active, thus, it will behave exactly like before, working with a grid consumption between ~ 100 and 700 W in this special case. I see this as a seperate concern at the moment. IMHO, we should first decide whether we want to limit the boost to ensure no grid consumption (it is a highly demanded feature in issues and discussions) and in case, proceed with this PR and then look at the maxAcPower case in an extra PR. |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="core/loadpoint.go" line_range="1532" />
<code_context>
// in MinPV mode or under special conditions return at least minCurrent
- if battery := batteryStart || batteryBuffered && lp.charging(); (mode == api.ModeMinPV || battery) && targetCurrent < minCurrent {
+ if battery := batteryStart || batteryBuffered && lp.charging() || lp.batteryBoost == boostContinue; (mode == api.ModeMinPV || battery) && targetCurrent < minCurrent {
lp.log.DEBUG.Printf("pv charge current: min %.3gA > %.3gA (%.0fW @ %dp, battery: %t)", minCurrent, targetCurrent, sitePower, activePhases, battery)
return minCurrent
</code_context>
<issue_to_address>
**question (bug_risk):** Extended battery condition mixes boost state into `battery` flag, which may affect non-MinPV modes
With `lp.batteryBoost == boostContinue` folded into `battery`, `battery` can now be true even when neither `batteryStart` nor `batteryBuffered` apply. Because the condition is `(mode == api.ModeMinPV || battery) && targetCurrent < minCurrent`, boost continuation will now force `minCurrent` in all modes, not just MinPV. If you intend `battery` to represent only battery-based conditions and boost to be treated separately, please split the flags or adjust the expression (and parentheses) to make the intended precedence and behavior explicit.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| if maxDischargePower := lp.site.GetBatteryMaxDischargePower(); maxDischargePower > 0 { | ||
| // limit delta to what the battery can still provide |
There was a problem hiding this comment.
shouldn't total discharge be limited to battery limited, not delta?
There was a problem hiding this comment.
A lot of logic happens outside. The boostPower function calculates an offset applied to sitePower to push battery demand, it is basically "how much power can I add as available for charging". Previously, the delta was always applied (11 kw when starting boost, 100+ when continuing). Now it caps the delta to the available remaining battery discharge power headroom.
But while thinking about it, I found a regression. Since the batteryBoostPower is set to 0 in the call to lp.update in site.go when it is negative (battery charging), the headroom is now limited compared to before when starting the boost. I will tink about this a bit more.
Will put it to draft again for now until fully understood.
There was a problem hiding this comment.
I feel there has to be a simpler solution when the maximum discharge power is known.
There was a problem hiding this comment.
But while thinking about it, I found a regression. Since the batteryBoostPower is set to 0 in the call to lp.update in site.go when it is negative (battery charging), the headroom is now limited compared to before when starting the boost. I will think about this a bit more.
This is now fixed. On the master and in the updated implementation, there is still an issue when the battery is charging while boost is running. In this case, it will only slowly increase the charge current, not considering the full available headroom.
….com/mfuchs1984/evcc into feat/battery_boost_maxdischargepower
|
Just waiting for a quick real-world test on my system after the last change. I can do that after im back from vacations in a bit more than a week. |
In #27756 and other discussions and issues, the wish to limit grid use when using battery boost came up. Currently, depending on charger + vehicle capabilities (mA control vs coarsecurrent) and the used residualpower, grid usage can go above about 700W.
This PR limits the maximum boost power to the aggregated maxDischargePower of the configured batteries. When one battery has no limit configured, the limit is not applied.
Limitations:
@andig this was mainly created by gemini. Let me know if you think this is a valuable enhancement. In case, I will start testing the changes on a production system.
Fix #29806