Planner: fix repeating overrun #26529
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- The new
planLockedTime/Soc/Idfields follow the same pattern as the existingplanActive/planSlotEndstate (which is already noted as not mutex-guarded); consider clearly documenting which callers must hold the lock (or centralizing access through lock-wrapped helpers) to avoid future data races around the lock/unlock logic. - Helper methods like
nextVehiclePlan,getPlanId, andclearPlanLockare now used both from locked and unlocked contexts; it would be safer either to make them consistently require the caller to hold the lock (and state that explicitly) or to add internal locking in these helpers to avoid accidental misuse.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `planLockedTime/Soc/Id` fields follow the same pattern as the existing `planActive`/`planSlotEnd` state (which is already noted as not mutex-guarded); consider clearly documenting which callers must hold the lock (or centralizing access through lock-wrapped helpers) to avoid future data races around the lock/unlock logic.
- Helper methods like `nextVehiclePlan`, `getPlanId`, and `clearPlanLock` are now used both from locked and unlocked contexts; it would be safer either to make them consistently require the caller to hold the lock (and state that explicitly) or to add internal locking in these helpers to avoid accidental misuse.
## Individual Comments
### Comment 1
<location> `core/loadpoint.go:998` </location>
<code_context>
}
- _, _, id := lp.NextVehiclePlan()
- return id > 1
+ return lp.getPlanId() > 1
}
</code_context>
<issue_to_address>
**issue (bug_risk):** repeatingPlanning now bypasses mutex protection that NextVehiclePlan previously provided
`repeatingPlanning` used to rely on `NextVehiclePlan`, which took an `RLock` before accessing plan state. After this change it calls `getPlanId`, which calls `nextVehiclePlan` without any locking, so reads of `planLocked*` and related fields can race unless `repeatingPlanning` is always called with the loadpoint lock held. Please either add locking inside `getPlanId` (as in `EffectivePlanId`), introduce a locked helper for `repeatingPlanning`, or make the lock requirement for `repeatingPlanning` explicit and ensure all call sites respect it.
</issue_to_address>
### Comment 2
<location> `core/loadpoint_plan.go:17-20` </location>
<code_context>
// TODO planActive is not guarded by mutex
+// clearPlanLock clears the locked plan goal
+func (lp *Loadpoint) clearPlanLock() {
+ lp.planLockedTime = time.Time{}
+ lp.planLockedSoc = 0
+ lp.planLockedId = 0
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** clearPlanLock mutates shared plan state without enforcing locking, which can lead to races at some call sites
This helper mutates shared fields (`planLockedTime`, `planLockedSoc`, `planLockedId`) without taking the loadpoint mutex. Although `ClearPlanLock` wraps it with `Lock/Unlock`, other callers (`setPlanActive`, `setPlanEnergy` – marked `no mutex` – and `evVehicleDisconnectHandler`) now depend on the caller to hold the lock, which is easy to misuse and can introduce data races. It would be safer to make the locking discipline explicit and consistent, e.g. by only exposing a locked wrapper and keeping this function `lock-required` (or splitting into `clearPlanLockLocked` plus a public, locking wrapper).
</issue_to_address>
### Comment 3
<location> `core/loadpoint_api.go:357-360` </location>
<code_context>
// setPlanEnergy sets plan target energy (no mutex)
func (lp *Loadpoint) setPlanEnergy(finishAt time.Time, energy float64) {
+ // clear locked goal when energy plan changes
+ lp.clearPlanLock()
+
lp.planEnergy = energy
</code_context>
<issue_to_address>
**suggestion (bug_risk):** setPlanEnergy is documented as "no mutex" but now calls clearPlanLock, which also assumes external locking
Because `setPlanEnergy` now mutates `planLocked*` via `clearPlanLock` without locking, callers must already ensure mutual exclusion for both `planEnergy` and the locked-plan fields. To prevent misuse, either update the "no mutex" note to reflect this requirement, provide a locked wrapper for callers without a lock, or move locking into `setPlanEnergy` and add a separate `setPlanEnergyLocked` for internal use.
```suggestion
// setPlanEnergy sets plan target energy and may modify plan lock state.
// Caller must ensure appropriate locking; no internal mutex is acquired here.
func (lp *Loadpoint) setPlanEnergy(finishAt time.Time, energy float64) {
// clear locked goal when energy plan changes
lp.clearPlanLock()
```
</issue_to_address>
### Comment 4
<location> `core/loadpoint_effective_test.go:130-128` </location>
<code_context>
}
}
+
+func TestPlanLocking(t *testing.T) {
+ clk := clock.NewMock()
+ now := clk.Now()
+
+ lp := NewLoadpoint(util.NewLogger("foo"), nil)
+ lp.clock = clk
+
+ planTime := now.Add(2 * time.Hour)
+
+ t.Run("lock and unlock", func(t *testing.T) {
+ lp.lockPlanGoal(planTime, 80, 2)
+
+ // locked values returned before plan target
+ ts, soc, id := lp.nextVehiclePlan()
+ assert.Equal(t, planTime, ts)
+ assert.Equal(t, 80, soc)
+ assert.Equal(t, 2, id)
+
+ clk.Add(3 * time.Hour) // advance past plan target
+
+ // locked values persist during overrun
+ ts, soc, id = lp.nextVehiclePlan()
+ assert.Equal(t, planTime, ts)
+ assert.Equal(t, 80, soc)
+ assert.Equal(t, 2, id)
+
+ // after clearing, lock is not returned
+ lp.clearPlanLock()
+ ts, soc, id = lp.nextVehiclePlan()
+ assert.True(t, ts.IsZero())
+ assert.Equal(t, 0, soc)
+ assert.Equal(t, 0, id)
+ })
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests that exercise plan locking via the public/planner paths and clearing in all expected lifecycle events
`TestPlanLocking` only covers `lockPlanGoal`/`clearPlanLock` and `nextVehiclePlan` in isolation, so it doesn’t validate that the full planner flow sets/clears locks correctly. To better guard against regressions, please add tests that:
- Run the SOC-based planner until a plan becomes active and confirm the lock is held (e.g. `EffectivePlanTime` / `EffectivePlanId` remain fixed even as time advances past the slot end).
- Verify the lock is cleared when each documented event occurs:
- `setPlanActive(false)` (plan completes or deactivates),
- vehicle disconnect handler (plan cleared on disconnect),
- user plan updates via `SetPlanSoc` / `SetRepeatingPlans` (through `vehicle.ClearPlanLocks` / `site.clearPlanLocks`),
- switching to an energy-based plan via `SetPlanEnergy`.
These can be subtests or separate tests and should demonstrate the original overrun/wrap-around bug is fixed end-to-end, not just in helper functions.
Suggested implementation:
```golang
func TestPlanLocking(t *testing.T) {
clk := clock.NewMock()
now := clk.Now()
lp := NewLoadpoint(util.NewLogger("foo"), nil)
lp.clock = clk
planTime := now.Add(2 * time.Hour)
t.Run("lock and unlock", func(t *testing.T) {
lp.lockPlanGoal(planTime, 80, 2)
// locked values returned before plan target
ts, soc, id := lp.nextVehiclePlan()
assert.Equal(t, planTime, ts)
assert.Equal(t, 80, soc)
assert.Equal(t, 2, id)
clk.Add(3 * time.Hour) // advance past plan target
// locked values persist during overrun
ts, soc, id = lp.nextVehiclePlan()
assert.Equal(t, planTime, ts)
assert.Equal(t, 80, soc)
assert.Equal(t, 2, id)
// after clearing, lock is not returned
lp.clearPlanLock()
ts, soc, id = lp.nextVehiclePlan()
assert.True(t, ts.IsZero())
assert.Equal(t, 0, soc)
assert.Equal(t, 0, id)
})
t.Run("soc planner holds lock during overrun", func(t *testing.T) {
// fresh loadpoint with same mock clock
lp := NewLoadpoint(util.NewLogger("planner-lock-overrun"), nil)
lp.clock = clk
// configure a simple SOC-based plan that becomes active at planTime
// and targets 80% SOC for plan id 1.
//
// NOTE: SetPlanSoc is expected to configure the planner; adjust
// arguments if the actual signature differs.
lp.SetPlanSoc(80, planTime)
// mark plan active (as the planner would once the plan starts)
lp.setPlanActive(true)
// allow time to progress beyond the plan slot end. The expected bug was
// that EffectivePlanTime / EffectivePlanId would wrap around or change
// after the slot, but the lock should keep them fixed.
initialEffectiveTime := lp.EffectivePlanTime()
initialEffectiveId := lp.EffectivePlanId()
assert.Equal(t, planTime, initialEffectiveTime)
assert.NotZero(t, initialEffectiveId)
// advance well past the plan end
clk.Add(3 * time.Hour)
// Effective plan should still reflect the locked goal, not wrap around
// to a new slot or clear itself while the plan remains active.
effTimeAfterOverrun := lp.EffectivePlanTime()
effIdAfterOverrun := lp.EffectivePlanId()
assert.Equal(t, initialEffectiveTime, effTimeAfterOverrun)
assert.Equal(t, initialEffectiveId, effIdAfterOverrun)
})
t.Run("lock cleared when plan deactivates", func(t *testing.T) {
lp := NewLoadpoint(util.NewLogger("planner-lock-deactivate"), nil)
lp.clock = clk
lp.SetPlanSoc(80, planTime)
lp.setPlanActive(true)
// lock should be set once plan is active
assert.False(t, lp.EffectivePlanTime().IsZero())
assert.NotZero(t, lp.EffectivePlanId())
// deactivating the plan should clear the lock
lp.setPlanActive(false)
ts, soc, id := lp.nextVehiclePlan()
assert.True(t, ts.IsZero())
assert.Equal(t, 0, soc)
assert.Equal(t, 0, id)
assert.True(t, lp.EffectivePlanTime().IsZero())
assert.Equal(t, 0, lp.EffectivePlanId())
})
t.Run("lock cleared on vehicle disconnect", func(t *testing.T) {
lp := NewLoadpoint(util.NewLogger("planner-lock-disconnect"), nil)
lp.clock = clk
lp.SetPlanSoc(80, planTime)
lp.setPlanActive(true)
// ensure lock is set
assert.False(t, lp.EffectivePlanTime().IsZero())
assert.NotZero(t, lp.EffectivePlanId())
// simulate vehicle disconnect; the disconnect handler is expected to
// clear plan locks (via vehicle.ClearPlanLocks).
//
// NOTE: adjust the disconnect invocation to match the actual handler,
// e.g. lp.OnVehicleDisconnect(), lp.handleVehicleDisconnect(), etc.
lp.handleVehicleDisconnect()
ts, soc, id := lp.nextVehiclePlan()
assert.True(t, ts.IsZero())
assert.Equal(t, 0, soc)
assert.Equal(t, 0, id)
assert.True(t, lp.EffectivePlanTime().IsZero())
assert.Equal(t, 0, lp.EffectivePlanId())
})
t.Run("lock cleared on SetPlanSoc update", func(t *testing.T) {
lp := NewLoadpoint(util.NewLogger("planner-lock-setplansoc"), nil)
lp.clock = clk
lp.SetPlanSoc(80, planTime)
lp.setPlanActive(true)
// initial lock
assert.False(t, lp.EffectivePlanTime().IsZero())
assert.NotZero(t, lp.EffectivePlanId())
// user changes the SOC plan; planner should clear existing lock via
// vehicle.ClearPlanLocks / site.clearPlanLocks.
newPlanTime := planTime.Add(4 * time.Hour)
lp.SetPlanSoc(60, newPlanTime)
ts, soc, id := lp.nextVehiclePlan()
assert.True(t, ts.IsZero())
assert.Equal(t, 0, soc)
assert.Equal(t, 0, id)
assert.True(t, lp.EffectivePlanTime().IsZero())
assert.Equal(t, 0, lp.EffectivePlanId())
})
t.Run("lock cleared on SetRepeatingPlans", func(t *testing.T) {
lp := NewLoadpoint(util.NewLogger("planner-lock-repeating"), nil)
lp.clock = clk
lp.SetPlanSoc(80, planTime)
lp.setPlanActive(true)
// ensure lock exists
assert.False(t, lp.EffectivePlanTime().IsZero())
assert.NotZero(t, lp.EffectivePlanId())
// applying repeating plans should clear locks via site.clearPlanLocks.
// NOTE: adjust the argument type for SetRepeatingPlans to match the
// actual repeating plan configuration type used in the codebase.
lp.SetRepeatingPlans(nil)
ts, soc, id := lp.nextVehiclePlan()
assert.True(t, ts.IsZero())
assert.Equal(t, 0, soc)
assert.Equal(t, 0, id)
assert.True(t, lp.EffectivePlanTime().IsZero())
assert.Equal(t, 0, lp.EffectivePlanId())
})
t.Run("lock cleared on SetPlanEnergy", func(t *testing.T) {
lp := NewLoadpoint(util.NewLogger("planner-lock-energy"), nil)
lp.clock = clk
lp.SetPlanSoc(80, planTime)
lp.setPlanActive(true)
// ensure lock exists
assert.False(t, lp.EffectivePlanTime().IsZero())
assert.NotZero(t, lp.EffectivePlanId())
// switching to an energy-based plan (e.g. kWh target) should clear
// SOC-based plan locks via vehicle.ClearPlanLocks / site.clearPlanLocks.
//
// NOTE: adjust arguments to SetPlanEnergy to match the actual
// signature (e.g. energy kWh, target time, etc.).
lp.SetPlanEnergy(10, planTime.Add(1*time.Hour))
ts, soc, id := lp.nextVehiclePlan()
assert.True(t, ts.IsZero())
assert.Equal(t, 0, soc)
assert.Equal(t, 0, id)
assert.True(t, lp.EffectivePlanTime().IsZero())
assert.Equal(t, 0, lp.EffectivePlanId())
})
}
```
The above tests assume the following public and internal APIs exist with roughly these signatures:
- `func (lp *Loadpoint) SetPlanSoc(targetSoc int, targetTime time.Time)`
- `func (lp *Loadpoint) SetRepeatingPlans(plans interface{})`
- `func (lp *Loadpoint) SetPlanEnergy(energyKWh float64, targetTime time.Time)`
- `func (lp *Loadpoint) EffectivePlanTime() time.Time`
- `func (lp *Loadpoint) EffectivePlanId() int`
- `func (lp *Loadpoint) setPlanActive(active bool)` (unexported but accessible from tests in the same package)
- `func (lp *Loadpoint) handleVehicleDisconnect()` (or equivalent vehicle disconnect handler)
- Existing helpers: `lockPlanGoal`, `clearPlanLock`, `nextVehiclePlan`.
To adapt this to your codebase, you may need to:
1. Adjust the method names and parameter types for `SetPlanSoc`, `SetRepeatingPlans`, `SetPlanEnergy`, and the disconnect handler call to match the actual implementations.
2. If `EffectivePlanTime` / `EffectivePlanId` are not present or have different names, update the assertions accordingly to use the effective planner state accessors you already expose.
3. If `SetRepeatingPlans` expects a concrete slice/struct type for plans, replace `nil` with a minimal valid value that triggers clearing of existing locks (e.g. an empty slice).
4. If the vehicle disconnect handler is triggered indirectly (e.g. via a connection state setter), replace `lp.handleVehicleDisconnect()` with the correct call that causes `vehicle.ClearPlanLocks` to be executed.
These adjustments will keep the intent: exercising plan locking through the full planner/public API, and asserting that locks are held during overruns and cleared on all relevant lifecycle events.
</issue_to_address>
### Comment 5
<location> `core/loadpoint_effective_test.go:139-148` </location>
<code_context>
+
+ planTime := now.Add(2 * time.Hour)
+
+ t.Run("lock and unlock", func(t *testing.T) {
+ lp.lockPlanGoal(planTime, 80, 2)
+
+ // locked values returned before plan target
+ ts, soc, id := lp.nextVehiclePlan()
+ assert.Equal(t, planTime, ts)
+ assert.Equal(t, 80, soc)
+ assert.Equal(t, 2, id)
+
+ clk.Add(3 * time.Hour) // advance past plan target
+
+ // locked values persist during overrun
+ ts, soc, id = lp.nextVehiclePlan()
+ assert.Equal(t, planTime, ts)
+ assert.Equal(t, 80, soc)
+ assert.Equal(t, 2, id)
+
+ // after clearing, lock is not returned
+ lp.clearPlanLock()
+ ts, soc, id = lp.nextVehiclePlan()
+ assert.True(t, ts.IsZero())
+ assert.Equal(t, 0, soc)
+ assert.Equal(t, 0, id)
+ })
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Extend the lock test to distinguish SOC-based from non-SOC-based planning behavior
To better mirror production and cover edge cases, consider extending the test with:
- A subtest where SOC-based planning is disabled or an energy-based plan is active, and you assert that the lock is not applied or has no effect.
- A subtest with a realistic repeating plan on a vehicle, where the planner activates that plan and you verify that `EffectivePlanTime` / `EffectivePlanId` continue to expose the locked goal even after advancing the mock clock beyond the original slot end.
This would validate lock semantics for both SOC-based and non-SOC-based modes and confirm the UI-facing values remain consistent under locking.
Suggested implementation:
```golang
func TestPlanLocking(t *testing.T) {
clk := clock.NewMock()
now := clk.Now()
lp := NewLoadpoint(util.NewLogger("foo"), nil)
lp.clock = clk
planTime := now.Add(2 * time.Hour)
t.Run("lock and unlock", func(t *testing.T) {
lp.lockPlanGoal(planTime, 80, 2)
// locked values returned before plan target
ts, soc, id := lp.nextVehiclePlan()
assert.Equal(t, planTime, ts)
assert.Equal(t, 80, soc)
assert.Equal(t, 2, id)
clk.Add(3 * time.Hour) // advance past plan target
// locked values persist during overrun
ts, soc, id = lp.nextVehiclePlan()
assert.Equal(t, planTime, ts)
assert.Equal(t, 80, soc)
assert.Equal(t, 2, id)
// after clearing, lock is not returned
lp.clearPlanLock()
ts, soc, id = lp.nextVehiclePlan()
assert.True(t, ts.IsZero())
assert.Equal(t, 0, soc)
assert.Equal(t, 0, id)
})
t.Run("lock has no effect for non-soc-based planning", func(t *testing.T) {
// arrange: disable SOC-based planning or enable an energy-based plan
// (exact configuration depends on existing planner APIs)
configureNonSocBasedPlanning(lp)
lp.lockPlanGoal(planTime, 80, 2)
// when SOC-based planning is disabled, nextVehiclePlan should *not* expose the lock
ts, soc, id := lp.nextVehiclePlan()
// expected behavior: either no plan at all or the non-SOC-based plan, but not the locked SOC goal
assert.NotEqual(t, planTime, ts, "non-SOC-based planning should not use plan lock timestamp")
assert.NotEqual(t, 80, soc, "non-SOC-based planning should not use plan lock SOC")
assert.NotEqual(t, 2, id, "non-SOC-based planning should not use plan lock ID")
lp.clearPlanLock()
})
t.Run("locked goal exposed via effective plan beyond slot end", func(t *testing.T) {
// arrange: configure a realistic repeating plan on a vehicle
// so that an SOC-based plan would normally end before now+4h
configureRepeatingSocPlan(lp, now, planTime)
// activate the repeating plan and lock the current goal
lp.lockPlanGoal(planTime, 80, 2)
// initial effective plan state reflects the lock
effTime := lp.EffectivePlanTime()
effID := lp.EffectivePlanID()
assert.Equal(t, planTime, effTime)
assert.Equal(t, 2, effID)
// advance mock clock beyond the original slot end
clk.Add(4 * time.Hour)
// even after the slot has "ended", the UI-facing effective values must still show the locked goal
effTime = lp.EffectivePlanTime()
effID = lp.EffectivePlanID()
assert.Equal(t, planTime, effTime, "effective plan time should remain locked beyond slot end")
assert.Equal(t, 2, effID, "effective plan id should remain locked beyond slot end")
// once the lock is cleared, effective values should reflect the underlying planner again
lp.clearPlanLock()
unlockedTime := lp.EffectivePlanTime()
unlockedID := lp.EffectivePlanID()
assert.NotEqual(t, planTime, unlockedTime, "effective plan time should revert after clearing lock")
assert.NotEqual(t, 2, unlockedID, "effective plan id should revert after clearing lock")
})
}
```
The new subtests assume a few helpers and methods that likely already exist but may have different names or signatures in your codebase:
1. **Non-SOC-based / energy-based configuration**
- Implement `configureNonSocBasedPlanning(lp *Loadpoint)` (or inline its logic) to:
- Disable SOC-based planning on the loadpoint, *or*
- Activate an energy-based plan that does not use SOC targets.
- The assertions in that subtest should be adapted to your actual semantics:
- If `nextVehiclePlan` returns the non-SOC plan, assert that its timestamp/SOC/ID match that plan rather than just “not equal to the lock”.
- If your planner returns a zero plan when SOC-based planning is disabled, keep the `IsZero`/`0` expectations instead.
2. **Repeating SOC plan configuration**
- Implement `configureRepeatingSocPlan(lp *Loadpoint, now time.Time, planTime time.Time)` to:
- Attach a realistic repeating SOC-based plan to the loadpoint / vehicle.
- Ensure the plan has a slot that covers `planTime` but ends *before* `now + 4h`, so advancing the mock clock by 4h moves time beyond the original slot’s end.
3. **Effective plan API names**
- Adjust `lp.EffectivePlanTime()` and `lp.EffectivePlanID()` calls to your actual API:
- If they are fields, use `lp.EffectivePlanTime` / `lp.EffectivePlanID`.
- If they are methods with different names (e.g. `lp.effectivePlanTime()` or `lp.EffectivePlan().Time`), update the test accordingly.
4. **Lock-aware behavior of `nextVehiclePlan`**
- Ensure `nextVehiclePlan` is the same helper the existing test uses to retrieve the planned slot / goal.
- If `nextVehiclePlan` itself uses different semantics in non-SOC mode, adjust the expectations in the "non-soc-based planning" subtest to match those semantics.
5. **Imports**
- Confirm that `assert` comes from the same package used in the rest of the test file (e.g. `github.com/stretchr/testify/assert`) and that `clock` / `util` are already imported; otherwise, add or fix the imports at the top of the file.
</issue_to_address>
### Comment 6
<location> `core/loadpoint_effective.go:64` </location>
<code_context>
return nil
}
-// NextVehiclePlan returns the next vehicle plan time, soc and id
-func (lp *Loadpoint) NextVehiclePlan() (time.Time, int, int) {
- lp.RLock()
</code_context>
<issue_to_address>
**issue (complexity):** Consider restructuring the planning API to centralize locking in a single public method and inline trivial helpers to avoid redundant indirection and duplicated synchronization logic.
You can reduce the extra indirection and duplicated locking by:
1. Splitting “compute plan (incl. lock honoring)” from “synchronize access”.
2. Having a single public entry point that manages locking.
3. Inlining `getPlanId`, since it’s just thin plumbing.
### 1. Split locked computation from public API
Turn the current `nextVehiclePlan` into a “locked” helper and reintroduce a single public wrapper that handles the lock:
```go
// nextVehiclePlanLocked returns the next vehicle plan time, soc, id.
// Assumes caller holds at least a read lock.
func (lp *Loadpoint) nextVehiclePlanLocked() (time.Time, int, int) {
// return locked plan if available
if lp.planLockedId > 0 {
return lp.planLockedTime, lp.planLockedSoc, lp.planLockedId
}
// calculate fresh plan
if v := lp.GetVehicle(); v != nil {
var plans []plan
// static plan
if planTime, soc := vehicle.Settings(lp.log, v).GetPlanSoc(); soc != 0 {
plans = append(plans, plan{Id: 1, Soc: soc, End: planTime})
}
// repeating plans
for index, rp := range vehicle.Settings(lp.log, v).GetRepeatingPlans() {
// ... existing logic unchanged ...
}
// calculate earliest required plan start
if plan := lp.nextActivePlan(lp.effectiveMaxPower(), plans); plan != nil {
return plan.End, plan.Soc, plan.Id
}
}
return time.Time{}, 0, 0
}
// NextVehiclePlan returns the next vehicle plan time, soc and id with locking.
func (lp *Loadpoint) NextVehiclePlan() (time.Time, int, int) {
lp.RLock()
defer lp.RUnlock()
return lp.nextVehiclePlanLocked()
}
```
This keeps the new “locked plan” behavior but centralizes concurrency concerns into `NextVehiclePlan`.
### 2. Simplify Effective* methods and remove `getPlanId`
With the above, `EffectivePlanSoc/Id/Time` can be simplified and consistent, and `getPlanId` can be inlined while still keeping all reads under a single lock where needed:
```go
// EffectivePlanSoc returns the soc target for the current plan
func (lp *Loadpoint) EffectivePlanSoc() int {
_, soc, _ := lp.NextVehiclePlan()
return soc
}
// EffectivePlanId returns the id for the current plan
func (lp *Loadpoint) EffectivePlanId() int {
lp.RLock()
defer lp.RUnlock()
if lp.socBasedPlanning() {
_, _, id := lp.nextVehiclePlanLocked()
return id
}
if lp.planEnergy > 0 {
return 1
}
// no plan
return 0
}
// EffectivePlanTime returns the effective plan time
func (lp *Loadpoint) EffectivePlanTime() time.Time {
lp.RLock()
defer lp.RUnlock()
if lp.socBasedPlanning() {
ts, _, _ := lp.nextVehiclePlanLocked()
return ts
}
ts, _ := lp.getPlanEnergy() // assumes getPlanEnergy expects caller to hold lock
return ts
}
```
This way:
- Locking is not duplicated across multiple callers of `nextVehiclePlan`.
- All three `Effective*` methods follow a consistent pattern.
- `getPlanId` is removed in favor of clearer, inlined logic while preserving the new behavior and improved locking.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| _, _, id := lp.NextVehiclePlan() | ||
| return id > 1 | ||
| return lp.getPlanId() > 1 |
Contributor
There was a problem hiding this comment.
issue (bug_risk): repeatingPlanning now bypasses mutex protection that NextVehiclePlan previously provided
repeatingPlanning used to rely on NextVehiclePlan, which took an RLock before accessing plan state. After this change it calls getPlanId, which calls nextVehiclePlan without any locking, so reads of planLocked* and related fields can race unless repeatingPlanning is always called with the loadpoint lock held. Please either add locking inside getPlanId (as in EffectivePlanId), introduce a locked helper for repeatingPlanning, or make the lock requirement for repeatingPlanning explicit and ensure all call sites respect it.
Comment on lines
+17
to
+20
| func (lp *Loadpoint) clearPlanLock() { | ||
| lp.planLockedTime = time.Time{} | ||
| lp.planLockedSoc = 0 | ||
| lp.planLockedId = 0 |
Contributor
There was a problem hiding this comment.
issue (bug_risk): clearPlanLock mutates shared plan state without enforcing locking, which can lead to races at some call sites
This helper mutates shared fields (planLockedTime, planLockedSoc, planLockedId) without taking the loadpoint mutex. Although ClearPlanLock wraps it with Lock/Unlock, other callers (setPlanActive, setPlanEnergy – marked no mutex – and evVehicleDisconnectHandler) now depend on the caller to hold the lock, which is easy to misuse and can introduce data races. It would be safer to make the locking discipline explicit and consistent, e.g. by only exposing a locked wrapper and keeping this function lock-required (or splitting into clearPlanLockLocked plus a public, locking wrapper).
Comment on lines
357
to
+360
| // setPlanEnergy sets plan target energy (no mutex) | ||
| func (lp *Loadpoint) setPlanEnergy(finishAt time.Time, energy float64) { | ||
| // clear locked goal when energy plan changes | ||
| lp.clearPlanLock() |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): setPlanEnergy is documented as "no mutex" but now calls clearPlanLock, which also assumes external locking
Because setPlanEnergy now mutates planLocked* via clearPlanLock without locking, callers must already ensure mutual exclusion for both planEnergy and the locked-plan fields. To prevent misuse, either update the "no mutex" note to reflect this requirement, provide a locked wrapper for callers without a lock, or move locking into setPlanEnergy and add a separate setPlanEnergyLocked for internal use.
Suggested change
| // setPlanEnergy sets plan target energy (no mutex) | |
| func (lp *Loadpoint) setPlanEnergy(finishAt time.Time, energy float64) { | |
| // clear locked goal when energy plan changes | |
| lp.clearPlanLock() | |
| // setPlanEnergy sets plan target energy and may modify plan lock state. | |
| // Caller must ensure appropriate locking; no internal mutex is acquired here. | |
| func (lp *Loadpoint) setPlanEnergy(finishAt time.Time, energy float64) { | |
| // clear locked goal when energy plan changes | |
| lp.clearPlanLock() |
Member
Author
There was a problem hiding this comment.
@andig can you please verify or add mutex locks if required?
Maschga
reviewed
Jan 8, 2026
| } | ||
|
|
||
| // clearPlanLocks clears locked plan goals for all loadpoints | ||
| func (site *Site) clearPlanLocks() { |
Collaborator
There was a problem hiding this comment.
Die Methode wird aufgerufen, wenn bei einem Loadpoint
- der SOC eines Plans geändert wird
- eine Änderung an den Repeating Plans vorgenommen wird
Angenommen es gibt zwei Loadpoints:
- Bei Loadpoint 1 gibt es aktuell einen Plan Overrun
- Bei Loadpoint 2 wird währenddessen eine Änderung wie oben beschrieben vorgenommen
Dann wird beim Loadpoint 1 der aktive Plan zurückgesetzt, weil es bei Loadpoint 2 Änderungen gab.
Das führt dazu, dass der überscheitende Plan bei Loadpoint 1 verworfen wird und dort der nächste Plan errechnet wird. Der gewünschte SOC wird bei Loadpoint 1 nicht mehr erreicht.
Wenn ich irgendwo einen Denkfehler habe, gebt gerne Bescheid.
Member
Author
There was a problem hiding this comment.
Yes, this is was my deliberate tradeoff to keep the complexity down. The "proper" solution would be to only clear lock for the appropriate loadpoint (which we dont easily know in this api call). We would have to iterate all lps and check be connected vehicle name. See also:
// note: could be optimized by only clearing plan lock of the relevant loadpoint
v.clearPlanLocks()
Collaborator
There was a problem hiding this comment.
this is was my deliberate tradeoff to keep the complexity down
Since this leads to a known bug, I would rather fix it now before users create bug reports.
We would have to iterate all lps and check be connected vehicle name.
This solution seems quite simple. But AFAIK the vehicle name isn't unique, right?
This would lead to the same behavior when multiple guest vehicles are connected to multiple charging points. :-/
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #24339
Fixes issue where repeating SOC-based plans prematurely switch to the next plan (e.g. wrap to next day) when charging takes longer than the initial time.
🔐 added a locking/unlocking mechanism for time/soc/id
🔓 remove lock on vehicle disconnect, plan finish and user interaction (vehicle plan change)
🚗 only active for soc based vehicle charging
TODO