Loadpoint: implement geofencing for vehicle detection#28312
Open
Dodi1968 wants to merge 20 commits intoevcc-io:masterfrom
Open
Loadpoint: implement geofencing for vehicle detection#28312Dodi1968 wants to merge 20 commits intoevcc-io:masterfrom
Dodi1968 wants to merge 20 commits intoevcc-io:masterfrom
Conversation
![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 3 issues, and left some high level feedback:
- Consider validating geofence input (e.g., latitude/longitude ranges and non‑negative radius) in
SetGeofenceConfig(and/or the UI) so obviously invalid values don’t silently get stored and used in detection. - The geofence distance calculation in
isVehicleAtHomeis a standalone Haversine implementation; extracting this into a small helper (with a named Earth radius constant) would make it easier to reuse and test independently. - Access to
lp.GeofenceinisVehicleAtHomeis currently unsynchronized; if this method can run concurrently withSetGeofenceConfig, consider adding appropriate locking or copying the config locally to avoid potential data races.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider validating geofence input (e.g., latitude/longitude ranges and non‑negative radius) in `SetGeofenceConfig` (and/or the UI) so obviously invalid values don’t silently get stored and used in detection.
- The geofence distance calculation in `isVehicleAtHome` is a standalone Haversine implementation; extracting this into a small helper (with a named Earth radius constant) would make it easier to reuse and test independently.
- Access to `lp.Geofence` in `isVehicleAtHome` is currently unsynchronized; if this method can run concurrently with `SetGeofenceConfig`, consider adding appropriate locking or copying the config locally to avoid potential data races.
## Individual Comments
### Comment 1
<location path="core/loadpoint_vehicle.go" line_range="308-317" />
<code_context>
+// isVehicleAtHome checks wether vehicle is at home (geofencing)
+// false: if vehicle position is known and outside the defined radius
+// true: in all other cases, even in cases of error or if position is unknown
+func (lp *Loadpoint) isVehicleAtHome(vehicle api.Vehicle) bool {
+ if !lp.Geofence.Enabled || vehicle == nil {
+ return true
+ }
+
+ vs, ok := vehicle.(api.VehiclePosition)
+ if !ok {
+ lp.log.DEBUG.Println("vehicle do not support position tracking")
+ return true
+ }
+
+ // vehicle position
+ lat1, lon1, err := vs.Position()
+ if err != nil {
+ lp.log.ERROR.Printf("vehicle position: %v", err)
+ return true
+ }
+
+ lp.log.DEBUG.Printf("vehicle position: lat %.4f, lon %.4f", lat1, lon1)
+
+ if lat1 == 0 && lon1 == 0 {
+ lp.log.WARN.Println("vehicle not sending position data")
+ return true
+ }
+
+ // loadpoint location
+ lat2 := lp.Geofence.Lat
+ lon2 := lp.Geofence.Lon
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider treating zero-valued loadpoint coordinates as "geofence disabled" to avoid accidental lock-outs.
If `lp.Geofence.Enabled` is true while `lp.Geofence.Lat`/`Lon` are left at their default `0` values, the distance is calculated to (0,0). For any vehicle with a valid position far from (0,0), `isVehicleAtHome` will return false and effectively block detection. Since both `evcc.dist.yaml` and the UI default to `lat: 0`/`lon: 0`, it’s easy for users to enable geofencing without setting coordinates and end up locked out. To harden this, either treat `Lat == 0 && Lon == 0` as “no geofence configured” (and return true, like when no vehicle position is available) or reject such a configuration at validation time.
</issue_to_address>
### Comment 2
<location path="core/loadpoint_vehicle.go" line_range="334-343" />
<code_context>
+ }
+
+ // loadpoint location
+ lat2 := lp.Geofence.Lat
+ lon2 := lp.Geofence.Lon
+
+ // Differences in radiant
+ dLat := (lat2 - lat1) * math.Pi / 180.0
+ dLon := (lon2 - lon1) * math.Pi / 180.0
+
+ lat1 = lat1 * math.Pi / 180.0
+ lat2 = lat2 * math.Pi / 180.0
+
+ // Haversine formular
+ a := math.Sin(dLat/2)*math.Sin(dLat/2) + math.Sin(dLon/2)*math.Sin(dLon/2)*math.Cos(lat1)*math.Cos(lat2)
+ c := 2 * math.Atan2(math.Sqrt(a), math.Sqrt(1-a))
+ distance := 6371 * c // earth radius: 6371 km
+
+ lp.log.DEBUG.Printf("vehicle distance from loadpoint: %.3fkm", distance)
+
+ return distance * 1e3 <= lp.Geofence.Radius
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Access to lp.Geofence here is unsynchronized and may race with configuration updates.
`SetGeofenceConfig` updates `lp.Geofence` under `lp.Lock()`, but `isVehicleAtHome` reads its fields without holding the same lock (unless the caller does so consistently), which can trigger a race under the Go race detector. Consider copying `lp.Geofence` under `RLock()` at the start of `isVehicleAtHome` and using that copy, or requiring callers to hold the appropriate lock before calling.
</issue_to_address>
### Comment 3
<location path="core/loadpoint_vehicle.go" line_range="313-322" />
<code_context>
+ return true
+ }
+
+ vs, ok := vehicle.(api.VehiclePosition)
+ if !ok {
+ lp.log.DEBUG.Println("vehicle do not support position tracking")
+ return true
+ }
+
+ // vehicle position
+ lat1, lon1, err := vs.Position()
+ if err != nil {
+ lp.log.ERROR.Printf("vehicle position: %v", err)
+ return true
+ }
+
+ lp.log.DEBUG.Printf("vehicle position: lat %.4f, lon %.4f", lat1, lon1)
+
+ if lat1 == 0 && lon1 == 0 {
+ lp.log.WARN.Println("vehicle not sending position data")
+ return true
</code_context>
<issue_to_address>
**nitpick (typo):** Fix small typos and wording issues in log and comment text for clarity.
In the comment and log message, please correct `wether` → `whether`, `formular` → `formula`, and `vehicle do not support position tracking` → `vehicle does not support position tracking` for clearer text.
Suggested implementation:
```golang
// isVehicleAtHome checks whether vehicle is at home (geofencing)
```
```golang
vs, ok := vehicle.(api.VehiclePosition)
if !ok {
lp.log.DEBUG.Println("vehicle does not support position tracking")
return true
}
```
```golang
formula
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
isVehicleAtHomeyou comment// lock valuesbut readlp.Geofencewithout actually taking a lock; consider usingGetGeofenceConfig()or adding anRLock/RUnlockto ensure thread-safe access consistent with the rest of the loadpoint settings. - Geofence validation logic (lat/lon range, radius, 0/0 check) is duplicated in
SetGeofenceConfigandisVehicleAtHome; consider extracting a small helper to keep the rules in one place and avoid divergence over time. - You treat lat/lon
(0,0)as invalid both for the geofence and for vehicle position; if users near that coordinate are a realistic scenario, you may want a more explicit "unset" indicator instead of overloading the coordinate value itself.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `isVehicleAtHome` you comment `// lock values` but read `lp.Geofence` without actually taking a lock; consider using `GetGeofenceConfig()` or adding an `RLock`/`RUnlock` to ensure thread-safe access consistent with the rest of the loadpoint settings.
- Geofence validation logic (lat/lon range, radius, 0/0 check) is duplicated in `SetGeofenceConfig` and `isVehicleAtHome`; consider extracting a small helper to keep the rules in one place and avoid divergence over time.
- You treat lat/lon `(0,0)` as invalid both for the geofence and for vehicle position; if users near that coordinate are a realistic scenario, you may want a more explicit "unset" indicator instead of overloading the coordinate value itself.
## Individual Comments
### Comment 1
<location path="core/loadpoint_vehicle.go" line_range="309" />
<code_context>
+// false: if vehicle position is available and outside the defined radius
+// true: in all other cases, even in cases of error or if position is is not available
+func (lp *Loadpoint) isVehicleAtHome(vehicle api.Vehicle) bool {
+ geofence := lp.Geofence // lock values
+
+ if !geofence.Enabled || vehicle == nil {
</code_context>
<issue_to_address>
**issue (bug_risk):** Accessing lp.Geofence without holding the loadpoint lock can introduce a data race.
The `// lock values` comment implies this should be a safe snapshot, but `lp.Geofence` is read without `RLock`. Other accessors (e.g. `GetGeofenceConfig`) use the lock, so if `SetGeofenceConfig` updates `lp.Geofence` concurrently this is a data race. Please either take `RLock`/`RUnlock` around this read or call `GetGeofenceConfig()` here to reuse its locking behavior.
</issue_to_address>
### Comment 2
<location path="core/loadpoint_vehicle.go" line_range="307" />
<code_context>
+// isVehicleAtHome checks whether vehicle is at home (geofencing)
+// false: if vehicle position is available and outside the defined radius
+// true: in all other cases, even in cases of error or if position is is not available
+func (lp *Loadpoint) isVehicleAtHome(vehicle api.Vehicle) bool {
+ geofence := lp.Geofence // lock values
</code_context>
<issue_to_address>
**nitpick (typo):** Minor typo in comment describing geofence behavior.
In the last line, `if position is is not available` has a duplicated `is`. Please change it to `if position is not available` for clarity.
```suggestion
// true: in all other cases, even in cases of error or if position is not available
```
</issue_to_address>
### Comment 3
<location path="core/loadpoint_vehicle.go" line_range="319" />
<code_context>
+ lat1 := geofence.Lat
+ lon1 := geofence.Lon
+
+ if (lat1 == 0 && lon1 == 0) || geofence.Radius < 0 || math.Abs(lat1) > 90 || math.Abs(lon1) > 180 {
+ lp.log.ERROR.Printf("invalid geofence settings: %+v", geofence)
+ return true
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring geofence handling by extracting shared validation and a pure distance-based “at home” function while simplifying the logging logic to reduce branching and keep the method focused.
You can reduce the added complexity without changing behavior by isolating validation and the pure “at home” computation, and simplifying the logging.
### 1. Extract geofence validation (avoid duplication)
Instead of validating geofence inline in `isVehicleAtHome`, extract a reusable helper that you can also use from `SetGeofenceConfig`:
```go
// shared, e.g. in same file or a small geofence.go
func validateGeofenceConfig(geofence Geofence) error {
if (geofence.Lat == 0 && geofence.Lon == 0) ||
geofence.Radius < 0 ||
math.Abs(geofence.Lat) > 90 ||
math.Abs(geofence.Lon) > 180 {
return fmt.Errorf("invalid geofence settings: %+v", geofence)
}
return nil
}
```
Then in `isVehicleAtHome`:
```go
if err := validateGeofenceConfig(geofence); err != nil {
lp.log.ERROR.Println(err)
return true
}
```
And in `SetGeofenceConfig` (or wherever you already validate), call the same helper instead of duplicating the rules.
### 2. Separate pure “at home” computation from policy
Right now `isVehicleAtHome` mixes policy (“on error, treat as home”), logging, and the core distance check. You can isolate the pure logic:
```go
// pure function: given a valid geofence and vehicle coords, is it at home?
func isAtHome(geofence Geofence, lat, lon float64) bool {
d := distance(geofence.Lat, geofence.Lon, lat, lon) * 1e3
return d <= geofence.Radius
}
```
Then keep `isVehicleAtHome` as a small wrapper encoding the “treat as home on error” policy:
```go
func (lp *Loadpoint) isVehicleAtHome(vehicle api.Vehicle) bool {
geofence := lp.Geofence // lock values
if !geofence.Enabled || vehicle == nil {
return true
}
if err := validateGeofenceConfig(geofence); err != nil {
lp.log.ERROR.Println(err)
return true
}
vs, ok := vehicle.(api.VehiclePosition)
if !ok {
lp.log.DEBUG.Println("vehicle does not support position tracking")
return true
}
lat2, lon2, err := vs.Position()
if err != nil {
lp.log.INFO.Printf("vehicle position: %v", err)
return true
}
if lat2 == 0 && lon2 == 0 {
lp.log.INFO.Println("vehicle position: not available")
return true
}
lp.log.DEBUG.Printf("vehicle position: lat %.4f, lon %.4f", lat2, lon2)
atHome := isAtHome(geofence, lat2, lon2)
// logging only, no policy here
lp.log.DEBUG.Printf(
"vehicle distance from loadpoint: %.1fm (radius: %vm, atHome=%v)",
distance(geofence.Lat, geofence.Lon, lat2, lon2)*1e3,
geofence.Radius,
atHome,
)
return atHome
}
```
This keeps:
- configuration validation in one place,
- a pure, easily testable `isAtHome`,
- the policy ("errors → treat as at home") explicit in a small wrapper.
### 3. Simplify comparator logging
You can remove the extra branching just to build `text` by logging `atHome` directly, as above:
```go
lp.log.DEBUG.Printf(
"vehicle distance from loadpoint: %.1fm (radius: %vm, atHome=%v)",
distance(geofence.Lat, geofence.Lon, lat2, lon2)*1e3,
geofence.Radius,
atHome,
)
```
This preserves debug info while reducing branches and keeping the method focused on the core logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- You currently validate the geofence config both when setting it (
SetGeofenceConfig) and again inisVehicleAtHome; consider validating once on write (including inrestoreSettings) soisVehicleAtHomecan assume a valid config and skip repeated checks. - In
isVehicleAtHome,GetGeofenceConfigacquires a lock and you then immediately revalidate the struct; if the struct is treated as immutable after validation, you could avoid revalidation and potentially simplify the locking/access pattern aroundGeofence.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You currently validate the geofence config both when setting it (`SetGeofenceConfig`) and again in `isVehicleAtHome`; consider validating once on write (including in `restoreSettings`) so `isVehicleAtHome` can assume a valid config and skip repeated checks.
- In `isVehicleAtHome`, `GetGeofenceConfig` acquires a lock and you then immediately revalidate the struct; if the struct is treated as immutable after validation, you could avoid revalidation and potentially simplify the locking/access pattern around `Geofence`.
## Individual Comments
### Comment 1
<location path="core/geofence.go" line_range="61-69" />
<code_context>
+
+// validate geofence settings
+func validateGeofenceConfig(geofence loadpoint.GeofenceConfig) error {
+ if geofence.Enabled {
+ if (geofence.Lat == 0 && geofence.Lon == 0) ||
+ geofence.Radius < 0 ||
+ math.Abs(geofence.Lat) > 90 ||
+ math.Abs(geofence.Lon) > 180 {
+ return fmt.Errorf("invalid geofence settings: %+v", geofence)
+ }
+ }
+ return nil
+}
+
</code_context>
<issue_to_address>
**question:** Clarify whether a zero radius should be considered a valid geofence configuration
`validateGeofenceConfig` rejects negative radii but permits `radius == 0` when enabled, which effectively requires the vehicle to be at the exact coordinates. Given the otherwise strict validation (e.g. rejecting 0/0 coords), please confirm whether zero should be considered invalid or coerced to a minimum positive radius to avoid issues from GPS jitter.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- SetGeofenceConfig currently logs and silently ignores invalid geofence configurations; consider returning an error up the call chain (e.g., from Apply/HTTP handler) so clients get explicit feedback when their change is rejected.
- isVehicleAtHome assumes the stored geofence config is valid; to be more defensive, you could either re-run validateGeofenceConfig there or handle obviously invalid coordinates/radius before using them in distance calculations.
- On the frontend, the geofence form only marks lat/lon/radius as required but does not constrain them to the same ranges as the backend (lat ∈ [-90,90], lon ∈ [-180,180], radius > 0); mirroring these constraints client-side would avoid users entering values that are later rejected server-side.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- SetGeofenceConfig currently logs and silently ignores invalid geofence configurations; consider returning an error up the call chain (e.g., from Apply/HTTP handler) so clients get explicit feedback when their change is rejected.
- isVehicleAtHome assumes the stored geofence config is valid; to be more defensive, you could either re-run validateGeofenceConfig there or handle obviously invalid coordinates/radius before using them in distance calculations.
- On the frontend, the geofence form only marks lat/lon/radius as required but does not constrain them to the same ranges as the backend (lat ∈ [-90,90], lon ∈ [-180,180], radius > 0); mirroring these constraints client-side would avoid users entering values that are later rejected server-side.
## Individual Comments
### Comment 1
<location path="core/geofence.go" line_range="55-64" />
<code_context>
+}
+
+// validate geofence settings
+func validateGeofenceConfig(geofence loadpoint.GeofenceConfig) error {
+ if geofence.Enabled {
+ if (geofence.Lat == 0 && geofence.Lon == 0) ||
+ geofence.Radius <= 0 ||
+ math.Abs(geofence.Lat) > 90 ||
+ math.Abs(geofence.Lon) > 180 {
+ return fmt.Errorf("invalid geofence settings: %+v", geofence)
+ }
+ }
+ return nil
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Invalid geofence configs are only logged and not propagated, which may mislead callers into assuming success.
Since `SetGeofenceConfig` only logs validation failures and returns, callers (e.g. the HTTP handler) can't tell that the config was rejected and still report success. Please propagate the validation error from `SetGeofenceConfig` up through the API layer so the HTTP handler can return an appropriate failure status/payload and clients can handle invalid configs correctly.
</issue_to_address>
### Comment 2
<location path="core/loadpoint/config.go" line_range="72" />
<code_context>
lp.SetBatteryBoostLimit(payload.BatteryBoostLimit)
lp.SetLimitEnergy(payload.LimitEnergy)
lp.SetLimitSoc(payload.LimitSoc)
+ lp.SetGeofenceConfig(payload.Geofence)
// TODO mode warning
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Applying `Geofence` unconditionally may overwrite existing settings when the field is omitted in the payload.
Because `payload.Geofence` is a value type, it’s applied even when the request JSON omits geofence settings (it becomes a zero-value struct). This unintentionally clears any existing geofence configuration. To avoid resetting stored settings when the client doesn’t specify geofence data, consider making `Geofence` a pointer or otherwise tracking field presence so only explicitly provided values overwrite existing ones.
Suggested implementation:
```golang
Thresholds ThresholdsConfig `json:"thresholds"`
Soc SocConfig `json:"soc"`
Geofence *GeofenceConfig `json:"geofence,omitempty"`
}
```
```golang
lp.SetBatteryBoostLimit(payload.BatteryBoostLimit)
lp.SetLimitEnergy(payload.LimitEnergy)
lp.SetLimitSoc(payload.LimitSoc)
if payload.Geofence != nil {
lp.SetGeofenceConfig(*payload.Geofence)
}
// TODO mode warning
```
Other parts of the codebase that:
1. Construct this config struct will need to be updated to assign `&GeofenceConfig{...}` instead of a value where appropriate.
2. Marshal or unmarshal JSON for this struct should work as-is, but any code assuming `Geofence` is non-nil now needs to handle the `nil` case.
3. If `SetGeofenceConfig` currently accepts a pointer instead of a value, adjust the call to `lp.SetGeofenceConfig(payload.Geofence)` inside the `if` block instead of dereferencing it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider moving
validateGeofenceConfigandisAtHomeinto theloadpointpackage (or another shared config/utility package) rather thancore, since they operate purely onloadpoint.GeofenceConfigand would be easier to reuse and test in that context. - In
isVehicleAtHome, invalid geofence settings currently emit an ERROR log on every call; since this is a user configuration problem that is already validated inSetGeofenceConfig, you might want to downgrade this to DEBUG/INFO and/or avoid revalidating on each call to reduce noisy logs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider moving `validateGeofenceConfig` and `isAtHome` into the `loadpoint` package (or another shared config/utility package) rather than `core`, since they operate purely on `loadpoint.GeofenceConfig` and would be easier to reuse and test in that context.
- In `isVehicleAtHome`, invalid geofence settings currently emit an ERROR log on every call; since this is a user configuration problem that is already validated in `SetGeofenceConfig`, you might want to downgrade this to DEBUG/INFO and/or avoid revalidating on each call to reduce noisy logs.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Implements #27621
and could help with this issue: #21701
Geofencing is a feature that improves the reliability of vehicle detection.
If a vehicle supports both automatic vehicle detection based on charging status and position tracking, the geofencing feature checks whether the vehicle is within the defined radius around the configured loadpoint location in addition to the charging status information.
If a vehicle does not support position tracking, geofencing will not affect the detection of that vehicle.
Geofencing can be configured as a loadpoint setting via evcc.yaml or ui.
Example configuration: