Loadpoint: fix reentrant locks #2 (#18669)

evcc-io · Feb 8, 2025 · f93ceff · f93ceff
1 parent a6abb0c
commit f93ceff
Show file tree

Hide file tree

Showing 9 changed files with 103 additions and 36 deletions.
diff --git a/core/loadpoint.go b/core/loadpoint.go
@@ -80,11 +80,11 @@ type Loadpoint struct {
 	lpChan   chan<- *Loadpoint // update requests
 	log      *util.Logger
 
-	// exposed public configuration
-	sync.RWMutex // guard status
-
-	vmu sync.RWMutex // guard vehicle
+	rwMutex      int64        // count reentrant RWMutex
+	sync.RWMutex              // guard status
+	vmu          sync.RWMutex // guard vehicle
 
+	// exposed public configuration
 	CircuitRef string `mapstructure:"circuit"` // Circuit reference
 	ChargerRef string `mapstructure:"charger"` // Charger reference
 	VehicleRef string `mapstructure:"vehicle"` // Vehicle reference
@@ -982,7 +982,7 @@ func (lp *Loadpoint) repeatingPlanning() bool {
 	if !lp.socBasedPlanning() {
 		return false
 	}
-	_, _, id := lp.nextVehiclePlan()
+	_, _, id := lp.NextVehiclePlan()
 	return id > 1
 }
 
@@ -1247,7 +1247,7 @@ func (lp *Loadpoint) pvScalePhases(sitePower, minCurrent, maxCurrent float64) in
 	// - https://github.com/evcc-io/evcc/issues/1572
 	// - https://github.com/evcc-io/evcc/issues/2230
 	// - https://github.com/evcc-io/evcc/issues/2613
-	measuredPhases := lp.getMeasuredPhases()
+	measuredPhases := lp.GetMeasuredPhases()
 	if phases > 0 && phases < measuredPhases {
 		if lp.chargerUpdateCompleted() && lp.phaseSwitchCompleted() {
 			lp.log.WARN.Printf("ignoring inconsistent phases: %dp < %dp observed active", phases, measuredPhases)
@@ -1287,7 +1287,7 @@ func (lp *Loadpoint) pvScalePhases(sitePower, minCurrent, maxCurrent float64) in
 		waiting = true
 	}
 
-	maxPhases := lp.maxActivePhases()
+	maxPhases := lp.MaxActivePhases()
 	target1pCurrent := powerToCurrent(availablePower, 1)
 	scalable = maxPhases > 1 && phases < maxPhases && target1pCurrent > maxCurrent
 
@@ -1352,13 +1352,13 @@ func (lp *Loadpoint) publishTimer(name string, delay time.Duration, action strin
 
 // boostPower returns the additional power that the loadpoint should draw from the battery
 func (lp *Loadpoint) boostPower(batteryBoostPower float64) float64 {
-	boost := lp.getBatteryBoost()
+	boost := lp.GetBatteryBoost()
 	if boost == boostDisabled {
 		return 0
 	}
 
 	// push demand to drain battery
-	delta := lp.effectiveStepPower()
+	delta := lp.EffectiveStepPower()
 
 	// start boosting by setting maximum power
 	if boost == boostStart {
@@ -1392,7 +1392,7 @@ func (lp *Loadpoint) pvMaxCurrent(mode api.ChargeMode, sitePower, batteryBoostPo
 	var scaledTo int
 	if lp.hasPhaseSwitching() && lp.phaseSwitchCompleted() {
 		scaledTo = lp.pvScalePhases(sitePower, minCurrent, maxCurrent)
-	} else if lp.getBatteryBoost() != boostDisabled {
+	} else if lp.GetBatteryBoost() != boostDisabled {
 		lp.log.DEBUG.Printf("!! pvScalePhases phasesSwitched: %v, %v", lp.phasesSwitched, time.Since(lp.phasesSwitched))
 	}
 

diff --git a/core/loadpoint/api.go b/core/loadpoint/api.go
@@ -144,7 +144,7 @@ type API interface {
 	SetDisableDelay(delay time.Duration)
 
 	// GetBatteryBoost returns the battery boost
-	GetBatteryBoost() bool
+	GetBatteryBoost() int
 	// SetBatteryBoost sets the battery boost
 	SetBatteryBoost(enable bool) error
 

diff --git a/core/loadpoint/mock.go b/core/loadpoint/mock.go
diff --git a/core/loadpoint_api.go b/core/loadpoint_api.go
@@ -466,18 +466,13 @@ func (lp *Loadpoint) SetDisableDelay(delay time.Duration) {
 	}
 }
 
-// getBatteryBoost returns the battery boost
-func (lp *Loadpoint) getBatteryBoost() int {
+// GetBatteryBoost returns the battery boost
+func (lp *Loadpoint) GetBatteryBoost() int {
 	lp.RLock()
 	defer lp.RUnlock()
 	return lp.batteryBoost
 }
 
-// GetBatteryBoost returns the battery boost
-func (lp *Loadpoint) GetBatteryBoost() bool {
-	return lp.getBatteryBoost() > 0
-}
-
 // setBatteryBoost returns the battery boost
 func (lp *Loadpoint) setBatteryBoost(boost int) {
 	lp.Lock()
@@ -635,11 +630,15 @@ func (lp *Loadpoint) SetMaxCurrent(current float64) error {
 
 // GetMinPower returns the min loadpoint power for a single phase
 func (lp *Loadpoint) GetMinPower() float64 {
+	lp.Lock()
+	defer lp.Unlock()
 	return Voltage * lp.effectiveMinCurrent()
 }
 
 // GetMaxPower returns the max loadpoint power taking vehicle capabilities and phase scaling into account
 func (lp *Loadpoint) GetMaxPower() float64 {
+	lp.Lock()
+	defer lp.Unlock()
 	return Voltage * lp.effectiveMaxCurrent() * float64(lp.maxActivePhases())
 }
 

diff --git a/core/loadpoint_effective.go b/core/loadpoint_effective.go
@@ -40,7 +40,7 @@ type plan struct {
 
 func (lp *Loadpoint) nextActivePlan(maxPower float64, plans []plan) *plan {
 	for i, p := range plans {
-		requiredDuration := lp.GetPlanRequiredDuration(float64(p.Soc), maxPower)
+		requiredDuration := lp.getPlanRequiredDuration(float64(p.Soc), maxPower)
 		plans[i].Start = p.End.Add(-requiredDuration)
 	}
 
@@ -56,6 +56,13 @@ func (lp *Loadpoint) nextActivePlan(maxPower float64, plans []plan) *plan {
 	return nil
 }
 
+// NextVehiclePlan returns the next vehicle plan time, soc and id
+func (lp *Loadpoint) NextVehiclePlan() (time.Time, int, int) {
+	lp.RLock()
+	defer lp.RUnlock()
+	return lp.nextVehiclePlan()
+}
+
 // nextVehiclePlan returns the next vehicle plan time, soc and id
 func (lp *Loadpoint) nextVehiclePlan() (time.Time, int, int) {
 	if v := lp.GetVehicle(); v != nil {
@@ -91,14 +98,14 @@ func (lp *Loadpoint) nextVehiclePlan() (time.Time, int, int) {
 
 // EffectivePlanSoc returns the soc target for the current plan
 func (lp *Loadpoint) EffectivePlanSoc() int {
-	_, soc, _ := lp.nextVehiclePlan()
+	_, soc, _ := lp.NextVehiclePlan()
 	return soc
 }
 
 // EffectivePlanId returns the id for the current plan
 func (lp *Loadpoint) EffectivePlanId() int {
 	if lp.socBasedPlanning() {
-		_, _, id := lp.nextVehiclePlan()
+		_, _, id := lp.NextVehiclePlan()
 		return id
 	}
 	if lp.planEnergy > 0 {
@@ -111,7 +118,7 @@ func (lp *Loadpoint) EffectivePlanId() int {
 // EffectivePlanTime returns the effective plan time
 func (lp *Loadpoint) EffectivePlanTime() time.Time {
 	if lp.socBasedPlanning() {
-		ts, _, _ := lp.nextVehiclePlan()
+		ts, _, _ := lp.NextVehiclePlan()
 		return ts
 	}
 
@@ -194,13 +201,15 @@ func (lp *Loadpoint) effectiveLimitSoc() int {
 	return 100
 }
 
-// effectiveStepPower returns the effective step power for the currently active phases
-func (lp *Loadpoint) effectiveStepPower() float64 {
+// EffectiveStepPower returns the effective step power for the currently active phases
+func (lp *Loadpoint) EffectiveStepPower() float64 {
 	return Voltage * float64(lp.ActivePhases())
 }
 
 // EffectiveMinPower returns the effective min power for the minimum active phases
 func (lp *Loadpoint) EffectiveMinPower() float64 {
+	lp.RLock()
+	defer lp.RUnlock()
 	return Voltage * lp.effectiveMinCurrent() * float64(lp.minActivePhases())
 }
 

diff --git a/core/loadpoint_mutex.go b/core/loadpoint_mutex.go
@@ -0,0 +1,34 @@
+package core
+
+import (
+	"sync/atomic"
+	"testing"
+)
+
+func (lp *Loadpoint) RLock() {
+	if testing.Testing() && atomic.AddInt64(&lp.rwMutex, 1) > 1 {
+		panic("reentrant RLock")
+	}
+	lp.RWMutex.RLock()
+}
+
+func (lp *Loadpoint) RUnlock() {
+	if testing.Testing() {
+		atomic.AddInt64(&lp.rwMutex, -1)
+	}
+	lp.RWMutex.RUnlock()
+}
+
+func (lp *Loadpoint) Lock() {
+	if testing.Testing() && atomic.AddInt64(&lp.rwMutex, 1) > 1 {
+		panic("reentrant Lock")
+	}
+	lp.RWMutex.Lock()
+}
+
+func (lp *Loadpoint) Unlock() {
+	if testing.Testing() {
+		atomic.AddInt64(&lp.rwMutex, -1)
+	}
+	lp.RWMutex.Unlock()
+}
diff --git a/core/loadpoint_phases.go b/core/loadpoint_phases.go
@@ -40,10 +40,15 @@ func (lp *Loadpoint) resetMeasuredPhases() {
 	lp.publish(keys.PhasesActive, lp.ActivePhases())
 }
 
-// getMeasuredPhases provides synchronized access to measuredPhases
-func (lp *Loadpoint) getMeasuredPhases() int {
+// GetMeasuredPhases provides synchronized access to measuredPhases
+func (lp *Loadpoint) GetMeasuredPhases() int {
 	lp.RLock()
 	defer lp.RUnlock()
+	return lp.getMeasuredPhases()
+}
+
+// getMeasuredPhases provides synchronized access to measuredPhases
+func (lp *Loadpoint) getMeasuredPhases() int {
 	return lp.measuredPhases
 }
 
@@ -60,6 +65,14 @@ func expect(phases int) int {
 // ActivePhases returns the number of expectedly active phases for the meter.
 // If unknown for 1p3p chargers during startup it will assume 3p.
 func (lp *Loadpoint) ActivePhases() int {
+	lp.Lock()
+	defer lp.Unlock()
+	return lp.activePhases()
+}
+
+// activePhases returns the number of expectedly active phases for the meter.
+// If unknown for 1p3p chargers during startup it will assume 3p.
+func (lp *Loadpoint) activePhases() int {
 	physical := lp.getPhases()
 	vehicle := lp.getVehiclePhases()
 	measured := lp.getMeasuredPhases()
@@ -75,20 +88,30 @@ func (lp *Loadpoint) ActivePhases() int {
 	return active
 }
 
-// minActivePhases returns the minimum number of active phases for the loadpoint.
-func (lp *Loadpoint) minActivePhases() int {
+// MinActivePhases returns the minimum number of active phases for the loadpoint.
+func (lp *Loadpoint) MinActivePhases() int {
 	lp.RLock()
-	configuredPhases := lp.configuredPhases
-	lp.RUnlock()
+	defer lp.RUnlock()
+	return lp.minActivePhases()
+}
 
+// minActivePhases returns the minimum number of active phases for the loadpoint.
+func (lp *Loadpoint) minActivePhases() int {
 	// 1p3p supported or limit 1p
-	if lp.hasPhaseSwitching() || configuredPhases == 1 {
+	if lp.hasPhaseSwitching() || lp.configuredPhases == 1 {
 		return 1
 	}
 
 	return lp.maxActivePhases()
 }
 
+// MaxActivePhases returns the maximum number of active phases for the loadpoint.
+func (lp *Loadpoint) MaxActivePhases() int {
+	lp.RLock()
+	defer lp.RUnlock()
+	return lp.maxActivePhases()
+}
+
 // maxActivePhases returns the maximum number of active phases for the loadpoint.
 func (lp *Loadpoint) maxActivePhases() int {
 	physical := lp.getPhases()
@@ -103,9 +126,7 @@ func (lp *Loadpoint) maxActivePhases() int {
 
 	// if 1p3p supported then assume configured limit or 3p
 	if lp.hasPhaseSwitching() {
-		lp.RLock()
 		physical = lp.configuredPhases
-		lp.RUnlock()
 	}
 
 	return min(expect(vehicle), expect(physical), expect(measured), expect(charger))

diff --git a/core/loadpoint_plan.go b/core/loadpoint_plan.go
@@ -48,7 +48,11 @@ func (lp *Loadpoint) remainingPlanEnergy(planEnergy float64) float64 {
 func (lp *Loadpoint) GetPlanRequiredDuration(goal, maxPower float64) time.Duration {
 	lp.RLock()
 	defer lp.RUnlock()
+	return lp.getPlanRequiredDuration(goal, maxPower)
+}
 
+// getPlanRequiredDuration is the estimated total charging duration
+func (lp *Loadpoint) getPlanRequiredDuration(goal, maxPower float64) time.Duration {
 	if lp.socBasedPlanning() {
 		if lp.socEstimator == nil {
 			return 0

diff --git a/server/http.go b/server/http.go
@@ -183,7 +183,7 @@ func (s *HTTPd) RegisterSiteHandlers(site site.API, valueChan chan<- util.Param)
 			"smartCost":            {"POST", "/smartcostlimit/{value:-?[0-9.]+}", floatPtrHandler(pass(lp.SetSmartCostLimit), lp.GetSmartCostLimit)},
 			"smartCostDelete":      {"DELETE", "/smartcostlimit", floatPtrHandler(pass(lp.SetSmartCostLimit), lp.GetSmartCostLimit)},
 			"priority":             {"POST", "/priority/{value:[0-9]+}", intHandler(pass(lp.SetPriority), lp.GetPriority)},
-			"batteryBoost":         {"POST", "/batteryboost/{value:[01truefalse]+}", boolHandler(lp.SetBatteryBoost, lp.GetBatteryBoost)},
+			"batteryBoost":         {"POST", "/batteryboost/{value:[01truefalse]+}", boolHandler(lp.SetBatteryBoost, func() bool { return lp.GetBatteryBoost() > 0 })},
 		}
 
 		for _, r := range routes {