Loadpoint: add solar share setting - #32104
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider validating/clamping
solarShareinSetSolarShare(or at the HTTP/MQTT handler level) to a sane range (e.g. [0,1]) so unexpected values cannot invert or otherwise break the derived enable/disable thresholds. - In
pvMaxCurrentyou callGetSolarShare(), which acquires the loadpoint lock from inside a core control path that typically accesses fields directly; it would be safer and cheaper to readlp.solarSharedirectly (or via a non-locking helper) to avoid potential lock-ordering issues and reduce contention. - The new test
TestPVSolarShareOverridesThresholdschanges the globalVoltagewithout restoring its previous value; capturing and deferring restoration of the original voltage would prevent cross-test interference.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider validating/clamping `solarShare` in `SetSolarShare` (or at the HTTP/MQTT handler level) to a sane range (e.g. [0,1]) so unexpected values cannot invert or otherwise break the derived enable/disable thresholds.
- In `pvMaxCurrent` you call `GetSolarShare()`, which acquires the loadpoint lock from inside a core control path that typically accesses fields directly; it would be safer and cheaper to read `lp.solarShare` directly (or via a non-locking helper) to avoid potential lock-ordering issues and reduce contention.
- The new test `TestPVSolarShareOverridesThresholds` changes the global `Voltage` without restoring its previous value; capturing and deferring restoration of the original voltage would prevent cross-test interference.
## Individual Comments
### Comment 1
<location path="assets/js/components/Loadpoints/SettingsModal.vue" line_range="331-340" />
<code_context>
+ solarShare() {
+ return this.loadpoint?.solarShare ?? null;
+ },
+ solarShareOptions() {
+ const percent = range(100, 0, -10).sort((a, b) => a - b);
+ return [
</code_context>
<issue_to_address>
**suggestion (bug_risk):** solarShareOptions mixes sorting and filtering in a way that is slightly confusing and may allow values beyond the intended 0–100 range.
`percent` is generated with `range(100, 0, -10).sort(...)` and then re-sorted after merging, which is redundant. More importantly, `insertSorted(percent, Math.round((this.solarShare ?? 0) * 100))` combined with `.filter((value) => value >= 0)` allows values >100 if `solarShare` ever exceeds 1. If the UI should only show 0–100%, clamp the inserted value (e.g. `Math.min(100, Math.max(0, ...))`) and drop the extra sort to keep the options predictable and within the intended range.
```suggestion
solarShareOptions() {
const percent = range(100, 0, -10).sort((a, b) => a - b);
const solarSharePercent = Math.min(
100,
Math.max(0, Math.round((this.solarShare ?? 0) * 100)),
);
return [
{ value: -1, name: this.$t("main.loadpointSettings.solarShare.off") },
...insertSorted(percent, solarSharePercent)
.filter((value) => value >= 0 && value <= 100)
.map((value) => ({ value, name: this.fmtPercentage(value) })),
];
},
```
</issue_to_address>
### Comment 2
<location path="core/loadpoint.go" line_range="1604" />
<code_context>
lp.log.DEBUG.Printf("pv charge current: %.3gA = %.3gA + %.3gA (%.0fW @ %dp)", targetCurrent, effectiveCurrent, deltaCurrent, sitePower, activePhases)
+ // solar share overrides the enable/disable thresholds when set (nil = inactive)
+ solarShare := lp.GetSolarShare()
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the solar-share aware enable/disable threshold calculations from pvMaxCurrent into dedicated helper methods to keep the function focused on sequencing rather than threshold logic.
You can reduce the added complexity by moving the solar-share aware threshold logic out of `pvMaxCurrent` into small helpers. That keeps the function focused on sequencing while preserving all behaviour.
### 1. Encapsulate disable threshold
```go
// helper on lp
func (lp *LoadPoint) pvDisableThreshold(solarShare *float64) float64 {
if solarShare == nil {
return lp.Disable.Threshold
}
return (*solarShare - 1) * lp.EffectiveMinPower()
}
```
Use it in `pvMaxCurrent`:
```go
solarShare := lp.GetSolarShare()
if mode == api.ModePV && lp.enabled && targetCurrent < minCurrent {
projectedSitePower := sitePower
if lp.hasPhaseSwitching() && !lp.phaseTimer.IsZero() {
projectedSitePower -= Voltage * minCurrent * float64(activePhases-1)
}
disableThreshold := lp.pvDisableThreshold(solarShare)
if projectedSitePower >= disableThreshold && !lp.vehicleClimateActive() {
lp.log.DEBUG.Printf("projected site power %.0fW >= %.0fW disable threshold", projectedSitePower, disableThreshold)
// ...
}
// ...
}
```
### 2. Encapsulate enable decision
```go
// helper on lp
func (lp *LoadPoint) pvEnableDecision(
solarShare *float64,
targetCurrent, minCurrent, sitePower float64,
) (threshold float64, shouldEnable bool) {
if solarShare != nil {
threshold = -*solarShare * lp.EffectiveMinPower()
return threshold, sitePower <= threshold
}
threshold = lp.Enable.Threshold
shouldEnable = (threshold == 0 && targetCurrent >= minCurrent) ||
(threshold != 0 && sitePower <= threshold)
return threshold, shouldEnable
}
```
Use it in `pvMaxCurrent`:
```go
if mode == api.ModePV && !lp.enabled {
enableThreshold, shouldEnable := lp.pvEnableDecision(solarShare, targetCurrent, minCurrent, sitePower)
if shouldEnable {
lp.log.DEBUG.Printf("site power %.0fW <= %.0fW enable threshold", sitePower, enableThreshold)
// ...
}
}
```
This keeps all current semantics (including solar share overrides) but removes ad-hoc recomputation and nested override logic from `pvMaxCurrent`, making it easier to follow and maintain.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| solarShareOptions() { | ||
| const percent = range(100, 0, -10).sort((a, b) => a - b); | ||
| return [ | ||
| { value: -1, name: this.$t("main.loadpointSettings.solarShare.off") }, | ||
| ...insertSorted(percent, Math.round((this.solarShare ?? 0) * 100)) | ||
| .filter((value) => value >= 0) | ||
| .sort((a, b) => a - b) | ||
| .map((value) => ({ value, name: this.fmtPercentage(value) })), | ||
| ]; | ||
| }, |
There was a problem hiding this comment.
suggestion (bug_risk): solarShareOptions mixes sorting and filtering in a way that is slightly confusing and may allow values beyond the intended 0–100 range.
percent is generated with range(100, 0, -10).sort(...) and then re-sorted after merging, which is redundant. More importantly, insertSorted(percent, Math.round((this.solarShare ?? 0) * 100)) combined with .filter((value) => value >= 0) allows values >100 if solarShare ever exceeds 1. If the UI should only show 0–100%, clamp the inserted value (e.g. Math.min(100, Math.max(0, ...))) and drop the extra sort to keep the options predictable and within the intended range.
| solarShareOptions() { | |
| const percent = range(100, 0, -10).sort((a, b) => a - b); | |
| return [ | |
| { value: -1, name: this.$t("main.loadpointSettings.solarShare.off") }, | |
| ...insertSorted(percent, Math.round((this.solarShare ?? 0) * 100)) | |
| .filter((value) => value >= 0) | |
| .sort((a, b) => a - b) | |
| .map((value) => ({ value, name: this.fmtPercentage(value) })), | |
| ]; | |
| }, | |
| solarShareOptions() { | |
| const percent = range(100, 0, -10).sort((a, b) => a - b); | |
| const solarSharePercent = Math.min( | |
| 100, | |
| Math.max(0, Math.round((this.solarShare ?? 0) * 100)), | |
| ); | |
| return [ | |
| { value: -1, name: this.$t("main.loadpointSettings.solarShare.off") }, | |
| ...insertSorted(percent, solarSharePercent) | |
| .filter((value) => value >= 0 && value <= 100) | |
| .map((value) => ({ value, name: this.fmtPercentage(value) })), | |
| ]; | |
| }, |
|
Die Bei jedem "solarShare" kleiner 100% kommt ein negativer Wert raus. |
|
Thanks — the complexity point is addressed in d3be10e: the solar-share aware threshold logic now lives in On the 🤖 Generated with Claude Code |
Adds a per-loadpoint solarShare setting that determines how much of the minimum charging power must come from solar. While set, it derives the PV enable and disable points and the legacy enable/disable thresholds are ignored: enable = -solarShare * effectiveMinPower disable = (solarShare - 1) * effectiveMinPower 100% starts only on full surplus, lower values accept grid power. The setting is inactive when unset, keeping existing threshold configurations working unchanged. Exposed via REST (POST/DELETE /api/loadpoints/<id>/solarshare), MQTT and a selector in the loadpoint settings dialog.
The generated State schema requires solarShare, but it was only published once set, so /api/state omitted it for unconfigured loadpoints and failed schema validation. Publish it at boot like smartCostLimit.
Replace the repeated -1 literal with a named SOLAR_SHARE_OFF constant, drop two redundant sorts, and keep a configured value selectable instead of filtering it out. Also compare against the sentinel instead of testing the sign, which sent DELETE for a negative share.
Move the solar-share aware enable/disable threshold logic out of pvMaxCurrent into pvDisableThreshold and pvEnableDecision, keeping the function focused on sequencing. No behaviour change.
|
This would replace UI-wise I'd prefer
|
Refs #6192, #14578. Reimplements the stale #16507 on current master.
Adds a per-loadpoint
solarSharesetting that determines how much of the minimum charging power must come from solar. While it is set, it derives the PV enable and disable points and the legacy enable/disable thresholds are ignored:100% starts only on full surplus, lower values accept a share of grid power. The setting is inactive when unset, so existing threshold configurations keep working unchanged — this does not remove the thresholds yet, it only supersedes them while active.
Exposed via REST, MQTT and a selector in the loadpoint settings dialog:
🤖 Generated with Claude Code