From f1069fb1edcb765ea79b8de2ef229ad81079d950 Mon Sep 17 00:00:00 2001 From: andig Date: Thu, 2 Jul 2026 18:31:56 +0200 Subject: [PATCH] SAIC: return a stored status only once (#31411) --- vehicle/saic/api.go | 72 +++++++++++++++++++++++----------------- vehicle/saic/api_test.go | 45 +++++++++++++++++++++++++ vehicle/saic/provider.go | 21 ++++++------ 3 files changed, 97 insertions(+), 41 deletions(-) create mode 100644 vehicle/saic/api_test.go diff --git a/vehicle/saic/api.go b/vehicle/saic/api.go index 4364eb1f7..c1a32570d 100644 --- a/vehicle/saic/api.go +++ b/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 } diff --git a/vehicle/saic/api_test.go b/vehicle/saic/api_test.go new file mode 100644 index 000000000..1f892754e --- /dev/null +++ b/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") + } +} diff --git a/vehicle/saic/provider.go b/vehicle/saic/provider.go index 1ea32f51b..f222ba69d 100644 --- a/vehicle/saic/provider.go +++ b/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)