Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

carwings async requests #1057

Merged
merged 16 commits into from
May 30, 2021
Merged

carwings async requests #1057

merged 16 commits into from
May 30, 2021

Conversation

maku1604
Copy link
Contributor

Change carwings requests to async, similar to ford and nissan async updates

@andig andig added the enhancement New feature or request label May 22, 2021
@andig
Copy link
Member

andig commented May 23, 2021

/rebase

return v.refreshResult()
}

if err = v.refreshRequest(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doppelt?

@andig
Copy link
Member

andig commented May 27, 2021

Ich habe die status Methode jetzt nochmal komplett neu aufgesetzt und vereinfacht. Was hälst du so davon?

Viel besser. Ich hab nochmal aufgeräumt- es waren noch einige Aktionen mehrfach im Code, tw. auch Altlasten. Magst Du's nochmal prüfen?

@maku1604
Copy link
Contributor Author

OK, ja ich schau heute Abend nochmal drauf. Danke!

@andig andig marked this pull request as ready for review May 27, 2021 06:17
@andig
Copy link
Member

andig commented May 27, 2021

Mich stören immer noch die erneuten Logins, bin aber nicht sicher ob die entfernt werden könnten? Im Prinzip würden sie noch an einer Stelle fehlen. Ggf. könnten wir das nochmal ein eine eigene Fehlerbehandlungsfunktion auslagern.

@maku1604
Copy link
Contributor Author

Ein Problem beim Carwings Server ist leider, dass der scheinbar nur eine Session pro User hält. D.H immer wenn man die Nissan App verwendet, muss sich evcc hinterher neu connecten und andersrum auch in der App muss man sich dauernd neu anmelden....

@andig
Copy link
Member

andig commented May 27, 2021

Na wenigstens lässt sich das Szenario gut testen :)

err = api.ErrMustRetry
} else {
// Update finished in the meantime, reset key for next request
v.refreshKey = ""
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich habe Fälle gesehen, in denen das Ergebnis zwischenzeitlich angekommen ist und elapsed dann nicht mehr über dem ExpiryTimeout war. Dann wurde der refreshKey nicht gelöscht und somit beim nächsten durchlauf erst nach einem veralteten Key gesucht. Ich versuche gerade das nochmal mit dem aktuellen Stand zu reproduzieren,

@maku1604
Copy link
Contributor Author

Mit allen anderen Änderungen ausser evtl. der oben kommentierten bin ich einverstanden. Praxistest läuft gerade.

@maku1604
Copy link
Contributor Author

Mit dem aktuellen Status bekomme ich nur noch 404 Fehler von Carwings. Ich tippe da ist was bei der Verschiebung von Session und Region im argen.

@andig
Copy link
Member

andig commented May 29, 2021

Hättest Du evtl Zugangsdaten für mich damit ich selbst mal rein schauen kann ([email protected])?

@maku1604
Copy link
Contributor Author

Ich schau mir das heute Abend nochmal an. Wollte nur erstmal Bescheid geben, dass der aktuelle Zustand nicht gemerged werden sollte.

@andig
Copy link
Member

andig commented May 30, 2021

Den refresh key verstehe ich, aber was ist mit dem Login? Egtl. Sollte doch errnotloggedin kommen wenn man nicht drin ist und der dann immer abgefangen werden. Die zusätzliche needLogin Variable gefällt mir gar nicht.

@maku1604
Copy link
Contributor Author

NeedLogin hat mir auch nicht wirklich gefallen. Ich habe das jetzt nochmal anders über das Fehler handling gelöst. Nachteil dieser Lösung ist, dass carwings bei jedem start von evcc erstmal bewusst falsch angesprochen wird und dann erst über das Error handling der login zustande kommt. Aber es funktioniert so.

@andig
Copy link
Member

andig commented May 30, 2021

Warum nicht einfach einmal am Anfang einloggen? So hattest Du es doch zwischendurch? Warum erkennt carnet selbst nicht die 404 als notloggedin?

@maku1604
Copy link
Contributor Author

Wenn am Anfang, dann nur ohne Error Detektion, sonst startet evcc nicht mehr sobald der Carwings server down ist. Den Login hattest du selbst im Dezember aus dem Grund schonmal von dort in die status Abfrage verschoben. PR #585.

@andig
Copy link
Member

andig commented May 30, 2021

Ahhh, verstehe. Ist blöd, dass man damit nicht user/pw prüfen kann, aber ist halt so. D.h. Jetzt gehts und rein damit?

@maku1604
Copy link
Contributor Author

Ich bin so jedenfalls zufrieden damit. Von mir aus rein damit.

@andig
Copy link
Member

andig commented May 30, 2021

Ich hab den Sonderfall mal hier dokumentiert joeshaw/carwings#36

}

err = api.ErrMustRetry
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Den verstehe ich noch nicht. Wenn es zum Timeout kommt, erzeuge ich beim ersten mal einen refreshRequest. Der wird irgendwann erfüllt und über refreshResult abgeholt. Im refreshResult wird der Key zurück gesetzt.

Warum ist das dann hier nochmal notwendig? Gibts da irgendwo noch einen Fehler in der Logik? Es riecht immer denn die Logik so verteilt ist nach versteckten Fehlern...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beim Batteriestatus request wird der timestamp der letzten Antwort gelesen. Ist jetzt seit dem letzten requestfinished mit result false die Antwort eingetrudelt, wird die so genommen und kein neuer Finish Check gemacht, da der elapsed wert nur weniger Sekunden hat=> der Key wird nicht gelöscht ohne den Else Zweig nicht gelöscht. Nach Ablauf des Cache (15min) wir dann der Finish Check mit dem alten Key gemacht... Returned success und damit bleiben die alten Werte weiter gecached anstatt neue zu requesten. Erst beim nächsten die Durchlauf (wieder 15 min) wird dann ein neuer Status request abgesetzt.

@andig andig merged commit e34877f into evcc-io:master May 30, 2021
@andig
Copy link
Member

andig commented May 30, 2021

Top, dann endlich rein damit. Vielen Dank für Deine Geduld!

@maku1604
Copy link
Contributor Author

So viel wie ich mit Go noch selbst am lernen bin, muss ich wohl eher dir danken für deine Geduld. 😁 Sollten wir carwings für Nissan Fahrzeuge mit Auslieferung vor 5/19 noch irgendwie in der Doku vermerken?

@andig
Copy link
Member

andig commented May 30, 2021

Gute Idee, mit cad5996 erledigt

@maku1604 maku1604 deleted the carwings-async branch May 30, 2021 20:11
@andig
Copy link
Member

andig commented Jun 16, 2021

@maku1604 ich glaube ich habe 2 Bugs gefunden:

  1. wenn es einen offenen RefreshRequest gibt, wird dennoch in StatusG immer zuerst die Batterie abgefragt. Das ist egtl. unnötig- hier sollte nur geprüft werden ob der Refresh erfolgreich war:

     if v.refreshKey != "" {
         if err := v.refreshResult(); err != nil {
             return err
         }
     }
    
     bs, err := v.session.BatteryStatus()
    
  2. der Climater fragt selbst statusG ab, hat aber intern ein anderes API. Der statusG is vmtl irrelevant, ich glaube der Refresh greift hier nicht.

Könntest Du Dir das nochmal anschauen?

@maku1604
Copy link
Contributor Author

OK, ich schau mir das nochmal an. Aus der Errinerung heraus, habe ich den Batteriestatus dort jeweils abgefragt um an das Alter der Daten zu kommen.

@andig
Copy link
Member

andig commented Jun 16, 2021

OK, siehe #1157

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants