compare vehicle status with charger status#29591
Conversation
|
PR in error state, no obvious progress. Lets reopen once clean. |
@andig it was marked as "draft" for a reason... I tested in the last days with real cars and adapted the testcases now -> please reopen |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider passing the charger status (e.g.,
lpStatus api.ChargeStatus) intoidentifyVehicleByStatusinstead of the fullloadpoint.APIto keep the function more focused and easier to reuse/test in isolation. - With the new strict
status == lp.GetStatus()condition, it might be worth thinking about how to handle transient mismatches (e.g., brief timing differences between vehicle and charger state) so that candidate identification does not become too brittle.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider passing the charger status (e.g., `lpStatus api.ChargeStatus`) into `identifyVehicleByStatus` instead of the full `loadpoint.API` to keep the function more focused and easier to reuse/test in isolation.
- With the new strict `status == lp.GetStatus()` condition, it might be worth thinking about how to handle transient mismatches (e.g., brief timing differences between vehicle and charger state) so that candidate identification does not become too brittle.
## Individual Comments
### Comment 1
<location path="core/coordinator/coordinator.go" line_range="149-150" />
<code_context>
- // vehicle is plugged or charging, so it should be the right one
- if status == api.StatusB || status == api.StatusC {
+ // vehicle is plugged or charging and has the same state as the charger, so it should be the right one
+ if (status == api.StatusB || status == api.StatusC) && status == lp.GetStatus() {
if res != nil {
c.log.WARN.Println("vehicle status: >1 matches, giving up")
</code_context>
<issue_to_address>
**suggestion (performance):** Cache the loadpoint status outside the loop to avoid repeated calls and potential inconsistency.
`lp.GetStatus()` is called for each vehicle during matching. If this is non-trivial or can change mid-loop, it adds overhead and makes the match timing-dependent. Read it once before the loop (e.g. `lpStatus := lp.GetStatus()`) and use `status == lpStatus` in the condition to keep the logic consistent and avoid repeated work.
Suggested implementation:
```golang
func (c *Coordinator) identifyVehicleByStatus(available []api.Vehicle, lp loadpoint.API) api.Vehicle {
var res api.Vehicle
c.mu.RLock()
lpStatus := lp.GetStatus()
```
```golang
// vehicle is plugged or charging and has the same state as the charger, so it should be the right one
if (status == api.StatusB || status == api.StatusC) && status == lpStatus {
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
suggestions from sourcery-ai make sense to me -> marked as draft, will include them tonight |
|
Really no need for comment noise- just do it ;) |
…s instead of loadpoint API
…to autodetect_compare_states
|
As after the review #29592 won't be merged in it's current state, this one does not have to wait anymore :) |
As mentioned numerous times before, this breaks with the inherently slow apis. |
|
@andig we already discussed that in slack? 🤔 I can see 3 possible scenarios:
I don't know, how relevant the precondition for case 3 (charging starts before detection is successful) is. When I asked for opinions on this tradeoff, your answer was "Gerne PR, Beides ist am Ende irgendwie falsch." So how to proceed from here? |
|
|
exactly compare charger status with vehicle status for identifying candidates instead of selecting all vehicles in states B and C
marked as draft as the Features are not yet tested with real cars and chargers, only in the evcc-demo with demo vehicles on my laptop