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

Fix SmartShunt energy values #756

Merged
merged 1 commit into from
Mar 17, 2024
Merged

Fix SmartShunt energy values #756

merged 1 commit into from
Mar 17, 2024

Conversation

PhilJaro
Copy link

Um Dezimalstellen für die charged und discharged Werte vom SmartShunt mit anzuzeigen, müssen die Datentypen angepasst werden, ich war mal so frei und habe das eben gemacht.

@schlimmchen
Copy link
Member

Nimm bitte ein float statt double.

Ich bin skeptisch. Seit wann überträgt der SmartShunt oder ein anderes VE.Direct-Gerät floats im Text-Protokoll? Hast du da einen Auszug aus einem Dokument von Victron, der das klar macht?

Ich vermute du hast das bei dir am laufen so? Und dann kommen Gleitkommezahlen wo raus? Im Live-View? Fehlt da nicht noch was wenn das vorher Integer waren? Dann muss ja kommuniziert werden, dass es jetzt Gleitkommazahlen sind und die müssen anständig gerundet werden...

@SW-Niko
Copy link

SW-Niko commented Mar 15, 2024

Die Daten (ich vermute H1-H3) kommen mit der Einheit mAh über die Schnittstelle.
grafik

Dann bleiben 3 Möglichkeiten:

  1. Nachkomma abschneiden und int lassen. Einheit [Ah]
  2. In float umwandeln Einheit [Ah]
  3. Beide Informationen weiterreichen und erst bei der weiteren Verwendung umrechnen

Aber @schlimmchen hat schon recht. wenn das nur in der veDirect Structur geändert wird, dann fallen die nachgelagerten Funktionen, die darauf zugreifen unter Umständen auf die Nase beziehungsweise die gutgemeinte Änderung hat keinen Effekt.
Wenn der Wert als int mit der Einheit mAh weitergereicht wird dann greifst du an der falschen Stelle an.

@PhilJaro
Copy link
Author

Habe mir das ganze jetzt nochmals angesehen und ja @schlimmchen du hast recht, Danke für den Hinweis.

Die Änderungen habe ich wieder Rückgängig gemacht und stattdessen in den BatteryStats an der Stelle wo das ganze auf kWh umgerechnet wird auf 100.0 angepasst, vorher reicht natürlich das int vollkommen aus.

Der LiveView ist bereits so vorbereitet, das er auf eine Nachkommastelle rundet, da stand bisher halt immer .0.
Per MQTT werden 2 Nachkommastellen bereitgestellt durch die Umrechnung auf kWh.

IMG_3715

@schlimmchen
Copy link
Member

schlimmchen commented Mar 16, 2024

Gerade habe ich gesehen, wie du vermutlich auf die ursprüngliche Umsetzung gekommen bist, Beispiel:

https://github.com/helgeerbe/OpenDTU-OnBattery/blob/7ebd4f46325961b9d3bb2392d99b33eff79ddb90/lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp#L243-L251

Das ist Mist so. Wenn Daten in mV (milli Volt) kommen, dann sollten sie auch so gespeichert werden. Es gibt überhaupt keinen Grund so weit unten schon an der Präzision zu fummeln. Verstehe ich nicht, warum das so gemacht wurde.

Deine neue Umsetzung ist jedenfalls sehr viel besser. Erlaube mir trotzdem, eine andere zu wählen, die nicht so subtil und implizit ist:

image

Stimmst du mir zu, dass es so klar ist, was erreicht werden soll?

Dass es mir / 100.0 statt / 100 überhaupt zum gewünschten Ergebnis kommt, wusste ich ehrlich gesagt noch nicht. Ich hatte gedacht, dass der erste Operand einer solchen Operation den Ergebnistyp bestimmt. 🤷‍♂️

Außerdem wäre es ja geschickt, wenn wir dann auch zwei Nachkommastellen in der GUI festlegen, und die Einheit reparieren:

image

@schlimmchen
Copy link
Member

@PhilJaro Kannst du den neuen Zustand deines Branches nochmal gegenchecken? Ich rechne damit, dass du nun im Live-View bein den Energiewerten des SmartShunt Kommastellen siehst, und zwar zwei. Wenn dem so ist, können wir das mergen.

Danke fürs finden und helfen! ❤️

@PhilJaro
Copy link
Author

PhilJaro commented Mar 16, 2024

@schlimmchen Ja, Du liegst richtig :D Ich hatte nachgesehen, wie das an anderer Stelle gelöst wurde und habe mich dann blind danach gerichtet. Es ist natürlich nich besonders klug das gleich von Beginn an umzuwandeln.
Aber nachdem ich mir dann den Code weiter verinnerlicht habe, ist mir das auch klar geworden. Habe das ganze erst seit ein paar tagen laufen, denke mit steigender verständinis des Codes meinerseits, werden meine PR's zukünfig durchdachter ;)

Zu Deiner Veränderung, das Ergebnis ist das gleiche, aber so ist es Verständlicher 👍🏻

Die 2 Nachkomma stellen werden nun ebenfalls wie von Dir erwartert im LiveView angezeigt:
image

@schlimmchen schlimmchen merged commit 21c19f4 into hoylabs:development Mar 17, 2024
8 checks passed
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants