SAIC: return a stored status only once (#31411)

vehicle/saic/api.go

@@ -19,16 +19,24 @@ const (
 	RegionAU = "https://gateway-mg-au.soimt.com/api.app/v1/"
 )
 
+// request states; a valid result implies no background poll is running
+type reqState int
+
+const (
+	stateInvalid reqState = iota // no pending value, no background poll
+	stateValid                   // a background value is pending, return once
+	stateRunning                 // background poll in progress
+)
+
 // API is an api.Vehicle implementation for SAIC cars
 type API struct {
 	*request.Helper
 	identity *Identity
 	log      *util.Logger
 
-	mu      sync.Mutex
-	running bool                  // background refresh in progress
-	valid   bool                  // result holds a valid response
-	result  requests.ChargeStatus // last valid response
+	mu     sync.Mutex
+	state  reqState
+	result requests.ChargeStatus // pending background response
 }
 
 // NewAPI creates a new vehicle
@@ -47,21 +55,28 @@ func NewAPI(log *util.Logger, identity *Identity) *API {
 	return v
 }
 
-// store saves a valid response and clears the running flag
+// store saves a background response to be returned exactly once
 func (v *API) store(res requests.ChargeStatus) {
 	v.mu.Lock()
 	defer v.mu.Unlock()
-	v.result, v.valid, v.running = res, true, false
+	v.result, v.state = res, stateValid
 }
 
-// last returns the last valid response, or ErrMustRetry until one is available
-func (v *API) last() (requests.ChargeStatus, error) {
+// take returns a pending value once, or ErrMustRetry while a poll runs. query
+// is true when neither applies and the caller must start a new request.
+func (v *API) take() (requests.ChargeStatus, error, bool) {
 	v.mu.Lock()
 	defer v.mu.Unlock()
-	if v.valid {
-		return v.result, nil
+
+	switch v.state {
+	case stateValid:
+		v.state = stateInvalid
+		return v.result, nil, false
+	case stateRunning:
+		return requests.ChargeStatus{}, api.ErrMustRetry, false
+	default:
+		return requests.ChargeStatus{}, nil, true
 	}
-	return v.result, api.ErrMustRetry
 }
 
 func (v *API) doRepeatedRequest(path string, event_id string) error {
@@ -88,20 +103,21 @@ func (v *API) doRepeatedRequest(path string, event_id string) error {
 
 // repeatRequest polls for the deferred answer in the background
 func (v *API) repeatRequest(path string, event_id string) {
-	// always clear running so a future query can start
-	defer func() {
-		v.mu.Lock()
-		v.running = false
-		v.mu.Unlock()
-	}()
-
 	for count := range 20 {
 		time.Sleep(2 * time.Second)
 		v.log.TRACE.Printf("repeated query %d", count)
+		// success stores the value (stateValid); a hard error stops the loop
 		if err := v.doRepeatedRequest(path, event_id); err != api.ErrMustRetry {
-			return
+			break
 		}
 	}
+
+	// reset so the next query starts fresh unless a value was stored
+	v.mu.Lock()
+	if v.state == stateRunning {
+		v.state = stateInvalid
+	}
+	v.mu.Unlock()
 }
 
 func doRequest[T any](v *API, req *http.Request, result *requests.Answer[T]) (string, error) {
@@ -176,13 +192,9 @@ func (v *API) Wakeup(vin string) error {
 func (v *API) Status(vin string) (requests.ChargeStatus, error) {
 	var zero requests.ChargeStatus
 
-	// refresh in progress, return last known value
-	v.mu.Lock()
-	running := v.running
-	v.mu.Unlock()
-	if running {
-		v.log.TRACE.Printf("query running")
-		return v.last()
+	// return a pending background value once, or keep retrying while a poll runs
+	if res, err, query := v.take(); !query {
+		return res, err
 	}
 
 	token, err := v.identity.Token()
@@ -209,7 +221,7 @@ func (v *API) Status(vin string) (requests.ChargeStatus, error) {
 
 	if event_id == "" {
 		v.log.TRACE.Printf("answer without event id")
-		return v.last()
+		return zero, api.ErrMustRetry
 	}
 
 	req, _ = requests.CreateRequest(
@@ -224,15 +236,15 @@ func (v *API) Status(vin string) (requests.ChargeStatus, error) {
 	// answer not yet available, keep polling in the background
 	if _, err = doRequest(v, req, &res); err == api.ErrMustRetry {
 		v.mu.Lock()
-		v.running = true
+		v.state = stateRunning
 		v.mu.Unlock()
 		v.log.TRACE.Printf("no answer yet, continuing in background")
 		go v.repeatRequest(path, event_id)
-		return v.last()
+		return zero, api.ErrMustRetry
 	} else if err != nil {
 		return zero, err
 	}
 
-	v.store(res.Data)
+	// fresh answer - returned directly, so it is consumed and not stored
 	return res.Data, nil
 }

vehicle/saic/api_test.go

@@ -0,0 +1,45 @@
+package saic
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/evcc-io/evcc/api"
+	"github.com/evcc-io/evcc/util"
+	"github.com/evcc-io/evcc/vehicle/saic/requests"
+)
+
+// TestTakeReturnsValueOnce verifies a stored value is returned exactly once,
+// and every other state yields ErrMustRetry or starts a fresh request.
+func TestTakeReturnsValueOnce(t *testing.T) {
+	v := &API{log: util.NewLogger("saic")}
+
+	// idle: no pending value and no poll -> caller must start a new query
+	if _, _, query := v.take(); !query {
+		t.Fatal("idle: expected query=true")
+	}
+
+	// background poll running -> ErrMustRetry, no new query, no value
+	v.mu.Lock()
+	v.state = stateRunning
+	v.mu.Unlock()
+	if _, err, query := v.take(); query || !errors.Is(err, api.ErrMustRetry) {
+		t.Fatalf("running: got err=%v query=%v, want ErrMustRetry", err, query)
+	}
+
+	// a background value arrives
+	var cs requests.ChargeStatus
+	cs.RvsChargeStatus.Mileage = 4242
+	v.store(cs)
+
+	// returned exactly once with nil error
+	res, err, query := v.take()
+	if query || err != nil || res.RvsChargeStatus.Mileage != 4242 {
+		t.Fatalf("first take: got mileage=%d err=%v query=%v, want value once", res.RvsChargeStatus.Mileage, err, query)
+	}
+
+	// consumed: the next call must start a fresh request again
+	if _, _, query := v.take(); !query {
+		t.Fatal("after consume: expected query=true")
+	}
+}

vehicle/saic/provider.go

@@ -54,8 +54,8 @@ func (v *Provider) Soc() (float64, error) {
 
 	val := res.ChrgMgmtData.BmsPackSOCDsp
 	if val > 1000 {
-		// v.status.Reset()
-		return float64(val), fmt.Errorf("invalid raw soc value: %d: %w", val, api.ErrMustRetry)
+		v.status.Reset()
+		return 0, fmt.Errorf("invalid raw soc value: %d: %w", val, api.ErrMustRetry)
 	}
 
 	return float64(val) / 10.0, nil
@@ -103,11 +103,13 @@ func (v *Provider) Range() (int64, error) {
 	if err != nil {
 		return 0, err
 	}
+
 	val := res.RvsChargeStatus.FuelRangeElec
-	if val < 10 {
-		// Ok, 0 would be possible, but it's more likely that it's an invalid answer.
-		return 0, api.ErrMustRetry
+	if val < 10 || val > 1000 {
+		v.status.Reset()
+		return 0, fmt.Errorf("invalid raw range value: %d: %w", val, api.ErrMustRetry)
 	}
+
 	return val / 10, nil
 }
 
@@ -127,19 +129,16 @@ var _ api.SocLimiter = (*Provider)(nil)
 
 // GetLimitSoc implements the api.SocLimiter interface
 func (v *Provider) GetLimitSoc() (int64, error) {
-	result := 0
 	res, err := v.status.Get()
 	if err != nil {
 		return 0, err
 	}
 
-	index := res.ChrgMgmtData.BmsOnBdChrgTrgtSOCDspCmd
-
-	if index <= Target100 {
-		result = TargetSocVals[res.ChrgMgmtData.BmsOnBdChrgTrgtSOCDspCmd]
+	if res.ChrgMgmtData.BmsOnBdChrgTrgtSOCDspCmd <= Target100 {
+		return int64(TargetSocVals[res.ChrgMgmtData.BmsOnBdChrgTrgtSOCDspCmd]), nil
 	}
 
-	return int64(result), err
+	return 0, api.ErrNotAvailable
 }
 
 var _ api.Resurrector = (*Provider)(nil)