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

Adjust code design more towards HA standards #56

Merged
merged 30 commits into from
May 19, 2024

Conversation

Xitee1
Copy link
Contributor

@Xitee1 Xitee1 commented Mar 17, 2024

Sadly because the keys changed this is a breaking change.
When an user updates, all his old entities will be unavailable and new ones will be generated and he can manually change the entity ids to the old ones if he wants to.
If we want to keep using the old unique ids to prevent this breaking change we would need to look for the old entity keys and use them instead. This isn't really hard but needs some work and we would remain using these (in my opinion) badly named keys that would also be used in the entity id then like before.

@Xitee1 Xitee1 marked this pull request as ready for review March 29, 2024 16:51
custom_components/sonnenbatterie/coordinator.py Outdated Show resolved Hide resolved
custom_components/sonnenbatterie/__init__.py Show resolved Hide resolved
custom_components/sonnenbatterie/sensor.py Show resolved Hide resolved
sensor_name, friendly_name, val, unit_name, device_class
)
# legacy support / prevent breaking changes (will not directly work this way because keys changed)
# return f"sensor.sonnenbatterie_{self.coordinator.serial}_{self.entity_description.key}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Siehe _atttr_has_entity_name weiter oben ...
Wenn geänderte Key-Namen unumgänglich sind, dann müssten wir da zumindest eine Liste mitliefern. Sonst denken die Leute nach dem Update ihre Integration ist kaputt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wie gesagt sind sie nicht unumgänglich, aber es wäre halt besser wenn sie einheitlicher benannt werden.
Wir können auch weiterhin die alten nehmen, dann muss ich die halt wieder alle umbenennen (in der Entity Liste und in den translations).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Naja, ich gehe halt davon aus, dass es Leute gibt, die bereits die alte Integration benutzen und ggf. darauf aufbauend auch Automatisierungen gebastelt haben. Da aus meiner Erfahrung niemand das Readme liest, sondern einfach nur auf Update geklickt wird und der nächste Weg dann sofort der zum Issue-Tracker ist, weil die Anzeige/Automatisierung/was-auch-sonst-immer nicht geht, denke ich es wäre netter gegenüber den Nutzern, nach Möglichkeit die alten Entity-Bezeichnungen zu behalten. Wenn es triftige technische Gründe für eine Änderung gibt, dann ist das wieder was anderes.
Letztlich ist das aber nicht meine Entscheidung, das darf der Chef machen @weltmeyer ;)

Copy link
Contributor Author

@Xitee1 Xitee1 Apr 8, 2024

Choose a reason for hiding this comment

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

Was wir auch machen könnten, wäre, für jeden Sensor ein weiteres Attribut legacy_key hinzuzufügen, das dann den alten key beinhalten würde, um den unique_key zu generieren um die existierenden entities zu behalten. Dadurch könnten wir intern weiterhin die neuen keys verwenden, die auch für die entity ids benutzt werden würden.
Und wer weiß, vielleicht gibt es ja irgendwann die Möglichkeit, unique keys zu migrieren.
Das würde eben ein paar zusätzliche Zeilen in der SENSORS Liste brauchen.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Und wer weiß, vielleicht gibt es ja irgendwann die Möglichkeit, unique keys zu migrieren. Das würde eben ein paar zusätzliche Zeilen in der SENSORS Liste brauchen.

Wenn ich mir https://community.home-assistant.io/t/replace-entities-and-keep-history-breaking-change-starting-ha-core-2023-4/639465 und https://community.home-assistant.io/t/how-to-replace-entity-in-energy-dashboard-by-new-one-without-losing-history/545933 so durchlese, dann fürchte ich, dass genau das jetzt nicht mehr geht. Für mich liest sich das so, als wären die alten unique_ids in Stein gemeisselt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aktuell spinnt meine Sonnenbatterie(Komme weder zuverlässig auf die API noch auf das lokale Dashboard... -.-),

Möglicherweise in letzter Zeit das LAN Kabel bewegt?
Meine Sonnenbatterie ging von Anfang an auch nur sehr unzuverlässig und langsam, bis ich irgendwann rausgefunden hab, dass die Techniker das Kabel ziemlich in die Kabelkanäle gequetscht haben und es dadurch kaputt gegangen ist. Mein Kabeltester hat wie wild durcheinander geblinkt anstatt in der Reihenfolge 1-8 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm also wenn durch das update als solches die Entity-IDs nicht verändert werden und die namen sich nur durch eine neu-installation verändern, dann würde ich das okay finden

Gut, dann werde ich das mit den legacy_keys machen.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@RustyDust hast du die Möglichkeit den PR als Update zu testen?

Wenn ich wieder Land sehe :/ Ich wollte das schon längst gemacht haben, aber die letzten Wochen waren die Hölle auf Arbeit ... ich schau wie ich dazu komme und melde mich.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, kein Problem. Ich glaube es wäre sowieso besser, wenn ich zuerst die legacy_keys mache, damit du dann das "finale" Update testen kannst.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Habe die Option hinzugefügt und bis auf diese Sensoren wird nun alles übernommen, inklusive Namen und Entity IDs.
ApplicationFrameHost_9YECvj50iC

@RustyDust
Copy link
Collaborator

Yay, ich bin endlich mal dazu gekommen, mit den PR anzusehen. Rein funktional sieht alles gut aus, allerdings zeigt mir HomeAssistant nach dem Update auf die PR-Version ein paar fehlende Entitäten an:

Bildschirmfoto 2024-05-19 um 15 43 47

Das ist halt insofern problematisch, als dass beim Nutzer keine Info erfolgt und gerade bei den Werten für "Production (in/out)" und "Operating Mode" die Wahrscheinlichkeit hoch ist, dass da Automatisierungen dranhängen. Kriegt man das noch irgendwie mit Alias geregelt?

@Xitee1
Copy link
Contributor Author

Xitee1 commented May 19, 2024

Das ist halt insofern problematisch, als dass beim Nutzer keine Info erfolgt und gerade bei den Werten für "Production (in/out)" und "Operating Mode" die Wahrscheinlichkeit hoch ist, dass da Automatisierungen dranhängen. Kriegt man das noch irgendwie mit Alias geregelt?

Was genau sollte Production in/out eigentlich anzeigen? Habe das nicht im Code gefunden und macht doch eigentlich kein Sinn, denn Produktion geht ja nur in eine Richtung (von den Solarzellen zum Netz, und nicht vom Netz zu den Solarzellen).

Der Operating Mode Sensor wurde aus den beiden Sensoren zusammengeführt. In der Oberfläche wird immer der Text angezeigt. Für Automatisierungen kann man den Numerischen Zustand nehmen (wird in Entwicklerwerkzeuge angezeigt). Deshalb gibt es den "(Text)" Sensor nicht mehr.
Falls da jetzt jemand eine Automatisierung damit gemacht hat, wird er wahrscheinlich eher den Numerischen Sensor genommen haben, als den mit dem Text (welchen es jetzt nicht mehr gibt). In dem Fall sollte dann weiterhin alles gehen.

Das der Temperatur-Sensor nicht mehr geht ist etwas komisch, denn dort habe ich als legacy_key "tmax" angegeben, was doch eigentlich der key vom alten Sensor sein sollte:

@RustyDust
Copy link
Collaborator

Was genau sollte Production in/out eigentlich anzeigen? Habe das nicht im Code gefunden und macht doch eigentlich kein Sinn, denn Produktion geht ja nur in eine Richtung (von den Solarzellen zum Netz, und nicht vom Netz zu den Solarzellen).

Wollte mal irgendwer haben, weil die Produktion tatsächlich manchmal negative Werte anzeigt (im kleinen ein- bis zweistelligen Watt-Bereich). Finde nur gerade den Screenshot nicht. Kann man denke ich vernachlässigen, hast Recht.

Der Operating Mode Sensor wurde aus den beiden Sensoren zusammengeführt. In der Oberfläche wird immer der Text angezeigt. Für Automatisierungen kann man den Numerischen Zustand nehmen (wird in Entwicklerwerkzeuge angezeigt). Deshalb gibt es den "(Text)" Sensor nicht mehr. Falls da jetzt jemand eine Automatisierung damit gemacht hat, wird er wahrscheinlich eher den Numerischen Sensor genommen haben, als den mit dem Text (welchen es jetzt nicht mehr gibt). In dem Fall sollte dann weiterhin alles gehen.

Ok. Sollten da Issues reinkommen können wir die ja an dich leiten ;)

Das der Temperatur-Sensor nicht mehr geht ist etwas komisch, denn dort habe ich als legacy_key "tmax" angegeben, was doch eigentlich der key vom alten Sensor sein sollte:

Der Alias geht ja auch ;) Wobei dieser Status - zumindest bei meiner SonnenBatterie - schon immer Quatsch war. Aktuell behauptet sie zum Beispiel, die Batterie hatte 0,31 °C. Also entweder war da schon immer was falsch oder da stimmt was mit der Berechnung nicht.

Copy link
Collaborator

@RustyDust RustyDust left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Owner

@weltmeyer weltmeyer left a comment

Choose a reason for hiding this comment

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

lgtm!

@weltmeyer weltmeyer merged commit 22483c9 into weltmeyer:master May 19, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants