diff --git a/vehicle/nissan/api.go b/vehicle/nissan/api.go index 34e8e5b48..2542da383 100644 --- a/vehicle/nissan/api.go +++ b/vehicle/nissan/api.go @@ -66,6 +66,18 @@ func (v *API) BatteryStatus(vin, version string) (StatusResponse, error) { var res StatusResponse err := v.GetJSON(uri, &res) + if err == nil { + res.Updated = time.Time{} + + if version == "v1" && res.LastUpdateTime != nil { + res.Updated = res.LastUpdateTime.Time + } + + if version == "v2" && res.Timestamp != nil { + res.Updated = time.Now() + } + } + return res, err } diff --git a/vehicle/nissan/api_test.go b/vehicle/nissan/api_test.go new file mode 100644 index 000000000..a298393f2 --- /dev/null +++ b/vehicle/nissan/api_test.go @@ -0,0 +1,96 @@ +package nissan + +import ( + "io" + "net/http" + "strings" + "testing" + "time" + + "github.com/evcc-io/evcc/util" + "github.com/evcc-io/evcc/util/request" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type roundTripFunc func(*http.Request) (*http.Response, error) + +func (f roundTripFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } + +func newTestAPI(body string) *API { + v := &API{Helper: request.NewHelper(util.NewLogger("test"))} + v.Client.Transport = roundTripFunc(func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(body)), + Header: http.Header{"Content-Type": {"application/json"}}, + }, nil + }) + return v +} + +// TestBatteryStatusV2IgnoresPastTimestamp verifies that BatteryStatus for v2 +// ignores the raw timestamp value and sets Updated to roughly now, regardless +// of how old the reported timestamp is. +// +// Background: the Nissan Ariya (Kamereon v2) returns timestamps that are +// 30 min–7 h in the past. Using that raw value as Updated caused Provider.status +// to always consider the result stale (time.Since > 5 min expiry) and issue +// an endless stream of refresh requests. Setting Updated = time.Now() on a +// non-nil timestamp breaks that loop. +func TestBatteryStatusV2IgnoresPastTimestamp(t *testing.T) { + past30m := time.Now().Add(-30 * time.Minute) + past2h := time.Now().Add(-2 * time.Hour) + past7h := time.Now().Add(-7 * time.Hour) + + cases := []struct { + name string + timestamp time.Time + }{ + {"30 minutes old", past30m}, + {"2 hours old (mid-range)", past2h}, + {"7 hours old (worst case)", past7h}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + body := `{"batteryLevel":80,"chargeStatus":1,"timestamp":"` + tc.timestamp.UTC().Format(time.RFC3339) + `"}` + res, err := newTestAPI(body).BatteryStatus("VIN", "v2") + require.NoError(t, err) + + // Updated must be close to now, not to the stale API timestamp. + assert.WithinDuration(t, time.Now(), res.Attributes.Updated, time.Minute, + "v2 Updated should be synthesized as now") + assert.False(t, res.Attributes.Updated.Equal(tc.timestamp), + "v2 must not use the raw past timestamp as Updated") + }) + } +} + +// TestBatteryStatusV2NoTimestampYieldsZero checks that a v2 response without +// a timestamp field leaves Updated as zero (no timestamp available at all). +func TestBatteryStatusV2NoTimestampYieldsZero(t *testing.T) { + body := `{"batteryLevel":80,"chargeStatus":1}` + res, err := newTestAPI(body).BatteryStatus("VIN", "v2") + require.NoError(t, err) + assert.True(t, res.Attributes.Updated.IsZero(), "v2 without timestamp should leave Updated zero") +} + +// TestBatteryStatusV1HonorsPastTimestamp contrasts v2 behaviour: v1 uses +// lastUpdateTime directly, so a past timestamp is preserved in Updated. +func TestBatteryStatusV1HonorsPastTimestamp(t *testing.T) { + past2h := time.Now().Add(-2 * time.Hour).UTC().Truncate(time.Second) + body := `{"batteryLevel":80,"chargeStatus":1,"lastUpdateTime":"` + past2h.Format(timeFormat) + `"}` + res, err := newTestAPI(body).BatteryStatus("VIN", "v1") + require.NoError(t, err) + assert.Equal(t, past2h, res.Attributes.Updated, "v1 should use the reported lastUpdateTime verbatim") +} + +// TestBatteryStatusV1NoTimestampYieldsZero checks that a v1 response without +// lastUpdateTime leaves Updated as zero. +func TestBatteryStatusV1NoTimestampYieldsZero(t *testing.T) { + body := `{"batteryLevel":80,"chargeStatus":1}` + res, err := newTestAPI(body).BatteryStatus("VIN", "v1") + require.NoError(t, err) + assert.True(t, res.Attributes.Updated.IsZero(), "v1 without lastUpdateTime should leave Updated zero") +} diff --git a/vehicle/nissan/provider.go b/vehicle/nissan/provider.go index 6107cc7fe..eaf5c5f25 100644 --- a/vehicle/nissan/provider.go +++ b/vehicle/nissan/provider.go @@ -43,7 +43,7 @@ func (v *Provider) status(battery func() (StatusResponse, error), refresh func() if err == nil { // result valid? - updated := res.Attributes.Updated() + updated := res.Attributes.Updated if time.Since(updated) < v.expiry || updated.IsZero() { v.refreshTime = time.Time{} return res, err @@ -145,7 +145,7 @@ func (v *Provider) FinishTime() (time.Time, error) { if res.Attributes.RemainingTime != nil { minutes := time.Duration(*res.Attributes.RemainingTime) * time.Minute - updated := res.Attributes.Updated() + updated := res.Attributes.Updated if !updated.IsZero() { return updated.Add(minutes), nil } diff --git a/vehicle/nissan/provider_test.go b/vehicle/nissan/provider_test.go new file mode 100644 index 000000000..e57430178 --- /dev/null +++ b/vehicle/nissan/provider_test.go @@ -0,0 +1,84 @@ +package nissan + +import ( + "testing" + "time" + + "github.com/evcc-io/evcc/api" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// batteryFunc returns a StatusResponse with the given Updated time and +// records how many times it was called. +func batteryFunc(updated time.Time) (func() (StatusResponse, error), *int) { + calls := 0 + return func() (StatusResponse, error) { + calls++ + res := StatusResponse{} + res.Attributes.Updated = updated + return res, nil + }, &calls +} + +// refreshFunc records calls and returns nil. +func refreshFunc() (func() (ActionResponse, error), *int) { + calls := 0 + return func() (ActionResponse, error) { + calls++ + return ActionResponse{}, nil + }, &calls +} + +// TestStatusV2SynthesizedNowSkipsRefresh is the core regression guard. +// +// When the Nissan Ariya (v2) API returns a timestamp 30 min–7 h in the past, +// BatteryStatus synthesizes Updated = time.Now(). Provider.status must treat +// that as fresh data (time.Since < 5 min expiry) and return the result +// immediately without calling the refresh endpoint. +func TestStatusV2SynthesizedNowSkipsRefresh(t *testing.T) { + p := &Provider{expiry: 5 * time.Minute} + + battery, batteryCalls := batteryFunc(time.Now()) + refresh, refreshCalls := refreshFunc() + + res, err := p.status(battery, refresh) + require.NoError(t, err) + assert.Equal(t, 1, *batteryCalls, "battery should be called once") + assert.Equal(t, 0, *refreshCalls, "refresh must not be called when data is fresh") + assert.Equal(t, 0, res.Attributes.BatteryLevel) +} + +// TestStatusZeroTimestampSkipsRefresh verifies that a zero Updated (no +// timestamp in the API response at all) is treated as valid, matching the +// `updated.IsZero()` branch in Provider.status. +func TestStatusZeroTimestampSkipsRefresh(t *testing.T) { + p := &Provider{expiry: 5 * time.Minute} + + battery, batteryCalls := batteryFunc(time.Time{}) + refresh, refreshCalls := refreshFunc() + + _, err := p.status(battery, refresh) + require.NoError(t, err) + assert.Equal(t, 1, *batteryCalls) + assert.Equal(t, 0, *refreshCalls, "refresh must not be called when Updated is zero") +} + +// TestStatusRawPastTimestampTriggersRefresh documents the bug that the +// timestamp fix resolved: if Updated held the raw Kamereon v2 timestamp +// (30 min–7 h old), time.Since(updated) > 5 min expiry, so every poll would +// trigger a refresh request. +func TestStatusRawPastTimestampTriggersRefresh(t *testing.T) { + p := &Provider{expiry: 5 * time.Minute} + + // Simulate what the old Updated() method returned for v2: the raw API timestamp. + past2h := time.Now().Add(-2 * time.Hour) + battery, batteryCalls := batteryFunc(past2h) + refresh, refreshCalls := refreshFunc() + + _, err := p.status(battery, refresh) + require.ErrorIs(t, err, api.ErrMustRetry, + "stale Updated (2 h old) should trigger a refresh and return ErrMustRetry") + assert.Equal(t, 1, *batteryCalls) + assert.Equal(t, 1, *refreshCalls, "refresh must be called when Updated is beyond expiry") +} diff --git a/vehicle/nissan/types.go b/vehicle/nissan/types.go index b0086860a..346fe54b3 100644 --- a/vehicle/nissan/types.go +++ b/vehicle/nissan/types.go @@ -92,17 +92,8 @@ type Attributes struct { // v2 Timestamp *time.Time `json:"timestamp"` BatteryAutonomy *int `json:"batteryAutonomy"` -} - -func (a *Attributes) Updated() time.Time { - if a.LastUpdateTime != nil { - // v1 - return a.LastUpdateTime.Time - } else if a.Timestamp != nil { - // v2 - return *a.Timestamp - } - return time.Time{} + // synthesized fields + Updated time.Time } type ActionResponse struct {