Repository: evcc-io/evcc
Pull request #2657 (2022-02-21 09:44:05)

Go-E Charger: Einfaches caching unnötiger API Statusabfragen innerhalb 2000ms
Habe es so umgebaut, wie es in der Cloud Abfrage bereits gemacht ist. Der Go-Echarger wird nur noch 1x abgefragt anstatt 8x innerhalb einer Sekunde. Der cache Wert wird im Api Konstruktor gesetzt und kann später von dort aus konfigurierbar gemacht werden, falls nötig. 2000ms scheint mir ein guter Wert, die 8 Anfragen sind nach 350ms bei mir abgearbeitet. Kommt wahrscheinlich auf die Komplexität des restlichen Setup an. Zur Vorsicht habe ich 2 TRACE Einträge zugefügt, um den Mechanismus beobachten zu können. Die können vielleicht später entfernt werden.

Ich bin nicht in Go zuhause, bitte seht es mir nach, wenn irgendwo ein style Fehler drin ist.

andig: Der Cache müsste mindestens invalidiert werden wenn irgendetwas umgeschaltet wird. Ich bin momentan noch nicht überzeugt, dass wir das ohne Not umsetzen wollen.
Waldmensch1: > Der Cache müsste mindestens invalidiert werden wenn irgendetwas umgeschaltet wird. Ich bin momentan noch nicht überzeugt, dass wir das ohne Not umsetzen wollen

nach meinen Beobachtungen vergehen zwischen 2 (Aktions-)Intervallen immer 10 Sekunden. Zwischen den Intervallen kommen nur mqtt subscriptions rein, auf die aber nicht reagiert wird. Nach meiner Beobachtung (allerdings hab ich nur den Go-E) wird im Fall von Umschaltungen "Update" benutzt, was eine eigene Response hat. Diese ist unabhängig vom Statusabruf und würde auch nicht gecached. Der Go-E macht innerhalb 1-2 Sekunden überhaupt nichts, was irgendwie auswertbar wäre. Man könnte den Intervall theoretisch auch auf 1000ms setzen. Das würde die 8 Abfragen, zumindest auf meinem System, auch noch einfangen. Beim Cloud Abruf ist der cache Wert irgendwie flexibel angelegt. Da habe ich aber die Spur nicht so wirklich rückverfolgen können, wo der tatsächlich als Wert gesetzt wird. Da fehlt mir die Kenntnis des kompletten Designs. Ich habe leider zu viel anderes Sachen auf dem Tisch, um mich tief einzuarbeiten, zumal ich bisher auch keine Berührungspunkte mit Go hatte.

Rein interessehalber, was ist bei dem Build check schiefgegangen? Sieht aus wie Probleme mit whitespaces.


andig: Deinem Quelltext fehlt die Behandlung mit `gofmt`.
Waldmensch1: Invalidierung ist jetzt mit drin, falls der Charger ge-updated wird. Selbe Methodik wie in der Cloud Api bereits umgesetzt ist
andig: > Hä? Im originalen Code wird oben nach v2 geprüft und Responsev2 geliefert und die Funktion innerhalb des If verlassen. Wenn V2 nicht zutrifft wurde der untere Code ausgeführt. Es wurde im Alten Code unterschieden und im neuen Code wird auch unterschieden. Warum es 2 unterschiedliche Responses gibt, weiß ich nicht. Das habe ich so nicht gebaut, das war schon vorher so.
Ich habe jetzt keinen Bock mehr, hier über Trivialitäten und leerzeilen zu diskutieren, für solche Spielchen ist mir echt die Zeit zu schade. Nimm es oder rejecte es. Ich nutze dann halt meinen Fork. Ich wollte nur unterstützen, aber sowas habe ich noch in keinem Open Source Projekt erlebt

Ich muss den Code noch warten wenn Du weg bist. Deshalb muss ich ihn verstehen. Kann gut sein, dass sich die Zusammenhänge im Github Diff UI nicht richtig erschlossen haben. 

Da Dir es aber anscheinend zuviel ist mich dabei zu unterstützen den PR zu verstehen und hier uaf eine persönliche Ebene kommen willst won‘tfix.
Waldmensch1: Originalcode
```
// Status reads a v1/v2 api response
func (c *LocalAPI) Status() (Response, error) {
	if c.v2 {
		res := new(StatusResponse2)
		err := c.response("status?filter=alw,car,eto,nrg,wh,trx,cards", &res)
		return res, err
	}

	res := new(StatusResponse)
	err := c.response("status", &res)
	return res, err
}
```

Mein Code

```
// Status reads a v1/v2 api response
func (c *LocalAPI) Status() (res Response, err error) {
	if time.Since(c.updated) > c.cache {
		if c.v2 {
			c.status = new(StatusResponse2)
			err = c.response("status?filter=alw,car,eto,nrg,wh,trx,cards", &c.status)
		} else {
			c.status = new(StatusResponse)
			err = c.response("status", &c.status)
		}
		if err == nil {
			c.updated = time.Now()
		}
	}
	return c.status, err
}
```

Wo ist der Unterschied? Beide Male wird nach v2 unterschieden. Das ist doch eindeutig. Wie gesagt, mir ist es egal. Lass mal den Thread nochmal Revue passieren und überlege, ob das so okay ist. Erst verlangst Du Typ Konformität bezüglich time.Duration. Dann halte ich mich dran in api_test.go - da ist es dann falsch und ich soll 0 nehmen. Dann stört eine Leerzeile. Wenn das der Umgang in diesem Projekt ist, wird es wohl nicht viele Leute geben, die auf so einen Spießrutenlauf Lust haben. Das hat auch nicht mit persönlicher Ebene zu tun. Ich bin im Beruf Senior Developer und mache normalerweise peer reviews. Wenn ich einen solchen Stil an den Tag legen würde, kämen wir nie voran.

Wahrscheinlich liest Du das eh nicht, aber es ist wenigstens dokumentiert für die Nachwelt
andig: > Erst verlangst Du Typ Konformität bezüglich time.Duration. Dann halte ich mich dran in api_test.go - da ist es dann falsch und ich soll 0 nehmen 

Hör Dir mal bitte selbst zu. Ich „verlange“ gar nichts. An den Code in diesem Repository habe ich aber Qualitätsansprüche. Dir musst Du nicht teilen. 

Da Go bei untypisierten integers selbst die Konvertierung in den Zieltyp macht war die time.Duration an der Stelle schlicht nicht notwendig.
andig: @Waldmensch1 wollen wirs nochmal miteinander versuchen und den PR mergen?
Waldmensch1: > @Waldmensch1 wollen wir's nochmal miteinander versuchen und den PR mergen?

Ich habe damit kein Problem damit, ansonsten hätte ich ihn zurückgezogen. Der PR ist okay und läuft, so wie er ist seit ein paar Tagen bei mir. Ich wüsste nicht, was noch dran geändert werden müsste. Ob man die V1 nun im else Zweig macht oder einfach am Ende macht, wenn die Funktion nicht vorher verlassen wurde, macht für mich keinen großen Unterschied.

Ich würde lediglich das **fwv** hier
`err = c.response("status?filter=alw,car,eto,nrg,wh,trx,cards", &c.status)`
noch anfügen, weil die Firmware Version bei StatusResponse2 m.E. nicht abgefragt wird, in den types2.go jedoch enthalten ist. In den letzten Tagen hat man gesehen, dass die FW Version zukünftig eventuell doch eine Rolle spielen könnte (Stichwort amx Parameter). Dann würde man vermutlich bei deren Abfrage und vorliegender V2 ein nil oder nullref kriegen. Das habe ich aber nicht getestet, weil ich evcc aktuell aus meinem Fork direkt auf dem Raspi gebaut und wieder dort laufen hab.

Patch:
diff --git a/.github/workflows/default.yml b/.github/workflows/default.yml
index b0d23fd5..f86e5982 100644
--- a/.github/workflows/default.yml
+++ b/.github/workflows/default.yml
@@ -29,7 +29,7 @@ jobs:
       uses: actions/cache@v2
       with:
         path: |
-          ~/go/pkg/mod
+          ~/go/pkg
           ~/.cache/go-build
         key: ${{ runner.os }}-${{ matrix.go-version }}-go-${{ hashFiles('**/go.sum') }}
         restore-keys: |
diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml
index 6d54b9ed..68947b6e 100644
--- a/.github/workflows/nightly.yml
+++ b/.github/workflows/nightly.yml
@@ -74,7 +74,7 @@ jobs:
       uses: actions/cache@v2
       with:
         path: |
-          ~/go/pkg/mod
+          ~/go/pkg
           ~/.cache/go-build
         key: ${{ runner.os }}-${{ matrix.go-version }}-go-${{ hashFiles('**/go.sum') }}
         restore-keys: |
diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
index 5517469c..3bc4edbf 100644
--- a/.github/workflows/release.yml
+++ b/.github/workflows/release.yml
@@ -54,7 +54,7 @@ jobs:
       uses: actions/cache@v2
       with:
         path: |
-          ~/go/pkg/mod
+          ~/go/pkg
           ~/.cache/go-build
         key: ${{ runner.os }}-${{ matrix.go-version }}-go-${{ hashFiles('**/go.sum') }}
         restore-keys: |
diff --git a/charger/go-e.go b/charger/go-e.go
index 60cfa18c..73921221 100644
--- a/charger/go-e.go
+++ b/charger/go-e.go
@@ -74,7 +74,7 @@ func NewGoE(uri, token string, cache time.Duration) (api.Charger, error) {
 	if token != "" {
 		c.api = goe.NewCloud(log, token, cache)
 	} else {
-		c.api = goe.NewLocal(log, util.DefaultScheme(uri, "http"))
+		c.api = goe.NewLocal(log, util.DefaultScheme(uri, "http"), cache)
 	}
 
 	if c.api.IsV2() {
diff --git a/charger/go-e/api.go b/charger/go-e/api.go
index df2c7a5b..9be034e3 100644
--- a/charger/go-e/api.go
+++ b/charger/go-e/api.go
@@ -32,19 +32,23 @@ type API interface {
 
 type LocalAPI struct {
 	*request.Helper
-	uri string
-	v2  bool
+	uri     string
+	v2      bool
+	status  Response
+	updated time.Time
+	cache   time.Duration
 }
 
 var _ API = (*LocalAPI)(nil)
 
-func NewLocal(log *util.Logger, uri string) *LocalAPI {
+func NewLocal(log *util.Logger, uri string, cache time.Duration) *LocalAPI {
 	uri = strings.TrimRight(uri, "/")
 	uri = strings.TrimSuffix(uri, "/api")
 
 	api := &LocalAPI{
 		Helper: request.NewHelper(log),
 		uri:    uri,
+		cache:  cache,
 	}
 
 	api.upgradeV2()
@@ -78,20 +82,25 @@ func (c *LocalAPI) response(partial string, res interface{}) error {
 }
 
 // Status reads a v1/v2 api response
-func (c *LocalAPI) Status() (Response, error) {
-	if c.v2 {
-		res := new(StatusResponse2)
-		err := c.response("status?filter=alw,car,eto,nrg,wh,trx,cards", &res)
-		return res, err
+func (c *LocalAPI) Status() (res Response, err error) {
+	if time.Since(c.updated) > c.cache {
+		if c.v2 {
+			c.status = new(StatusResponse2)
+			err = c.response("status?filter=alw,car,eto,nrg,wh,trx,cards", &c.status)
+		} else {
+			c.status = new(StatusResponse)
+			err = c.response("status", &c.status)
+		}
+		if err == nil {
+			c.updated = time.Now()
+		}
 	}
-
-	res := new(StatusResponse)
-	err := c.response("status", &res)
-	return res, err
+	return c.status, err
 }
 
 // Update executes a v1/v2 api update and returns the response
 func (c *LocalAPI) Update(payload string) error {
+	c.updated = time.Time{}
 	res := new(UpdateResponse)
 
 	if c.v2 {


Test patch:
diff --git a/charger/go-e/api_test.go b/charger/go-e/api_test.go
index b96ebc9c..e92363d7 100644
--- a/charger/go-e/api_test.go
+++ b/charger/go-e/api_test.go
@@ -37,7 +37,7 @@ func TestLocalV1(t *testing.T) {
 	srv := httptest.NewServer(h)
 
 	// h.expect("/api/status?filter=alw")
-	local := NewLocal(util.NewLogger("foo"), srv.URL)
+	local := NewLocal(util.NewLogger("foo"), srv.URL, 0)
 
 	h.expect("/status")
 	if _, err := local.Status(); err != nil {
@@ -54,7 +54,7 @@ func TestLocalV2(t *testing.T) {
 	srv := httptest.NewServer(h)
 
 	h.expect("/api/status?filter=alw")
-	local := NewLocal(util.NewLogger("foo"), srv.URL)
+	local := NewLocal(util.NewLogger("foo"), srv.URL, 0)
 
 	h.expect("/api/status?filter=alw,car,eto,nrg,wh,trx,cards")
 	if _, err := local.Status(); err != nil {