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

Bosch bpts5 hybrid #3029

Merged
merged 18 commits into from
Apr 3, 2022
Merged

Bosch bpts5 hybrid #3029

merged 18 commits into from
Apr 3, 2022

Conversation

waldtraut1981
Copy link
Contributor

Added support of PV battery system Bosch BPT-S 5 Hybrid

@premultiply premultiply added the devices Specific device support label Mar 28, 2022
@andig
Copy link
Member

andig commented Mar 28, 2022

Danke für Deinen PR! Bis Du Dir sicher, dass:

  • das Singleton Muster (=mit einer instance) hier passt? Bist Du sicher, dass es nur ein solches Gerät geben kann?
  • Ist es notwendig, den WR laufend zu pollen oder würde es nicht reichen, bei Bedarf abzufragen? Falls ja findet sich ein Vorschlag fürs Caching beim go-e Charger

Letztlich ist es bei Go üblich, Fehler nur 1x zu behandeln- entweder loggen oder zurück geben. Wenn Du Beides machst werden sie doppelt gelogt was nicht notwendig ist (Hinweis: fmt.Errorf um Loginfos zu ergänzen).

@andig andig marked this pull request as draft March 29, 2022 08:47
@waldtraut1981
Copy link
Contributor Author

Hi,

  • ich habe das Logging angepasst (sorry, bin noch etwas neu in Go).
  • Habe den lokalen request loop entsprechend dem Caching Pattern bei go-e Charger umgebaut. Hoffe das passt so besser.
  • Das Singleton für den API-Client hatte ich genutzt, da ich nicht für die unterschiedlichen Instanzen des Meters für grid, battery und pv einzelne Abrufe gegenüber dem Gerät haben wollte. Das führte leider teilweise dazu, dass meine PV-Anlage in einen Fehlerzustand wechselte. Daher jeweils alles basierend auf einem einzelnen Abruf des Geräts. Ich habe es jetzt noch so ergänzt, dass man prinzipiell mehrere Bosch BPT-S 5 Hybrid für eine evcc Instanz haben könnte. Jedoch halte ich es für sehr unwahrscheinlich, dass ein Nutzer mehrere dieser Solarwechselrichter mit integrierter Batterie betreibt.

@andig
Copy link
Member

andig commented Mar 29, 2022

Das Singleton für den API-Client hatte ich genutzt, da ich nicht für die unterschiedlichen Instanzen des Meters für grid, battery und pv einzelne Abrufe gegenüber dem Gerät haben wollte.

das ist ne Menge Code. Liegts denn wirklich an mehreren Verbindungen (das sind ja auch nur Http Requests?) oder einfach an der Häufigkeit der Zugriffe oder einer Wartezeit dazwischen? Häufigkeit hättest Du ja max. 3x parallel. Wartezeit lässt sich ganz einfach über ein time.Ticker machen.

ich würde den Code gerne deutlich vereinfachen bevor wir den einchecken…

@waldtraut1981
Copy link
Contributor Author

Leider kann ich aufgrund des closed sources des Bosch Systems nicht sagen, was dort zum Fehler führt. Ich hatte nur beobachtet, dass bei mehreren Connections und häufigeren Anfragen das PV-System dazu neigt in einen Fehlerzustand zu wechseln, der die gesamte Stromproduktion lahm legt und einen Neustart der gesamten Anlage erfordert. Der genutzte Endpoint des Systems ist hier ja auch nicht wirklich als offener Standard (wie z. B. JSON) zum Datenabruf gedacht, sondern eigentlich nur für die Darstellung der Übersicht auf einer Webseite, den ich hier abgreife. Ich gehe davon aus, dass hier die Umsetzung im Gerät nicht für die permanente und häufige Abfrage konzipiert ist. Der meiste Code ist denke ich für das parsing des speziellen Rückgabewertes. Gerne können wir hier weiter optimieren.
Im aktuellem Zustand läuft das System so bei mir schon die letzten drei Monate ganz gut.

@andig
Copy link
Member

andig commented Mar 30, 2022

Ich hab auch einen Schwung Vereinfachung ergänzt, da geht aber noch mehr.

Leider kann ich aufgrund des closed sources des Bosch Systems nicht sagen, was dort zum Fehler führt.

Könntest Du das probieren? Der aktuelle Code ist immer noch sehr komplex und v.a. racy- wenn wir auf parallele Zugriffe umstellen wird das nicht mehr funktionieren.

Rate limiting kannst Du ganz einfach so machen:

limiter := time.NewTicket(time.Second).C

Dann vor jedem HTTP Request einfach

<-limiter

Damit kannst du das ganze Singleton sparen bzw. auf einen einfachen Channel reduzieren. Auch das API kann dann entfallen.

@andig
Copy link
Member

andig commented Mar 30, 2022

Bei

currentTotalEnergy

ist auch noch was faul. Das wird nie aktualisiert.

@waldtraut1981
Copy link
Contributor Author

Hi,
der Decorator für TotalEnergy war hier ein Copy&Paste Fehler (hatte als Vorlage ein anderes Meter genommen). Aber die Bosch liefert mir leider keine kWh Werte sondern nur aktuelle kW Werte. Daher habe ich diesen Teil entfernt.
Habe die aktuelle Variante gebaut und getestet. Läuft.
Sorry, dass du bei meinem Code noch so viel Hand anlegen musst...

Das mit dem limiter versuche ich in den nächsten Tagen nochmal auszuprobieren.

@andig
Copy link
Member

andig commented Mar 30, 2022

Cool- wäre Klasse wenn Du das ausprobieren könntest. Dann wirds noch einfacher wenn das API verschwindet.

@waldtraut1981
Copy link
Contributor Author

waldtraut1981 commented Mar 31, 2022

Hi,
leider habe ich nochmal eine blöde Frage, weil ich mir das mit dem Rate-Limiting und dem Channel nochmal anschaue:
Es werden ja für pv, grid & battery jeweils drei eigenständige Instanzen der Meter-Implementierung erzeugt, oder? Wenn ich jetzt ein Rate-Limiting in den Code für die HTTP-Requests für die Instanzen einfüge, dann würde ich doch nur die Häufigkeit der Anfragen pro Zeiteinheit pro Instanz beschränken, oder irre ich mich da? Mit dem aktuellem Singleton des Request-Clients verindere ich jedoch zusätzlich noch, dass nicht drei mal (wegen der drei Instanzen) die gleiche Status-Abfrage durchgeführt wird. Würde ich hier ein Rate-Limiting einfügen, dann würden ja trotzdem die drei Anfragen ausgeführt, jedoch auf einen größeren Zeitraum verteilt. Oder kann der Channel von allen Instanzen zusammen verwendet werden?
Zudem würde jede Instanz bei einem eigenen Login eine eigenständige WUI SID erhalten (also drei unterschiedliche). Leider kann ich nicht genau sagen, was das Bosch System auf lange Nutzungszeit der HTTP Requests in die Knie zwingt. Ich habe hier aber auch die Nutzung von mehreren "Clients" mit unterschiedlichen WUI SIDs im Verdacht.

@waldtraut1981
Copy link
Contributor Author

habe jetzt das singleton und die API entfernt (alles wieder in die bosch_bpts5_hybrid umgezogen von der API. Baue das gleich mal und lasse es laufen. Bin gespannt, ob sich die Bosch beschwert und wann.

@waldtraut1981
Copy link
Contributor Author

habe jetzt das singleton und die API entfernt (alles wieder in die bosch_bpts5_hybrid umgezogen von der API. Baue das gleich mal und lasse es laufen. Bin gespannt, ob sich die Bosch beschwert und wann.

Nach ca. 6h Betriebszeit antwortete die Bosch nach dem Umbau (ohne Singleton) leider erst mit einem i/o timeout->no route to host->und anschließend mit einem 400er Error beim Versuch des Relogins :-(.
Leider so, wie ich es zu Beginn meiner Implementierungen auch häufiger beobachtet habe.

@andig
Copy link
Member

andig commented Apr 2, 2022

Gut. Dann git reset auf den commit vorher und dann lassen wi das so 👍🏻

@andig
Copy link
Member

andig commented Apr 2, 2022

Oder besser git revert!

Make member method
@waldtraut1981
Copy link
Contributor Author

Oder besser git revert!

done!

@andig andig marked this pull request as ready for review April 2, 2022 17:06
@andig
Copy link
Member

andig commented Apr 2, 2022

LGTM. Können wir Namen und Logger vielleicht noch auf

bosch-bpt

verkürzen?

@waldtraut1981
Copy link
Contributor Author

LGTM. Können wir Namen und Logger vielleicht noch auf

bosch-bpt

verkürzen?

Klar. Gerne.

@andig andig merged commit 786c470 into evcc-io:master Apr 3, 2022
@andig
Copy link
Member

andig commented Apr 3, 2022

Ist drin- vielen Dank :)

@waldtraut1981
Copy link
Contributor Author

Super! Gerne. Mein nächstes Projet ist eine WearOS App, die die Daten von der evcc-API auf der Smart-Watch anzeigen kann :)

@andig
Copy link
Member

andig commented Apr 3, 2022

Woah... dann wirds aber langsam Zeit, dass Du Dich bei Slack anmeldest!

@waldtraut1981
Copy link
Contributor Author

Woah... dann wirds aber langsam Zeit, dass Du Dich bei Slack anmeldest!

Also der Link unter https://docs.evcc.io/docs/Home ist hier leider nicht mehr valide: "Der Link ist nicht mehr aktiv"

@waldtraut1981 waldtraut1981 deleted the bosch_bpts5_hybrid branch April 3, 2022 15:03
@andig
Copy link
Member

andig commented Apr 3, 2022

/cc @naltatis könntest Du nochmal helfen?

@naltatis
Copy link
Member

naltatis commented Apr 3, 2022

/cc @naltatis könntest Du nochmal helfen?

Schräg. Ich hatte nach Ablauf unseres Slack Pro Trails extra noch mal die Invite Links überprüft. Da funktionierten die noch.
Ich aktualisiere die mal ....

@naltatis
Copy link
Member

naltatis commented Apr 4, 2022

@waldtraut1981 Die Slack-Invite Links sind nun aktualisiert. Versuch gerne noch mal.

dontbyte pushed a commit to dontbyte/evcc that referenced this pull request Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devices Specific device support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants