-
-
Notifications
You must be signed in to change notification settings - Fork 69
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
Minor VeDirect Improvements (parse additional values, ... ) #840
Conversation
SW-Niko
commented
Apr 3, 2024
•
edited
Loading
edited
- VeDirect: add of text message values "IL", "AR" and "MON" Fix issue [Request] Read more SmartShunt Text values #834
- VeDirect: discard BMV and Async MPPT History Data
Bei verboseLogging = off Und dann ist mir noch aufgefallen das diese Abfrage nicht mehr ganz richtig ist, weil in _lastByteMillis die Hex Bytes und die Text (Frame) Bytes vermischt werden. Das korrigiere ich auch noch.
|
Das finde ich nicht so gut. Im Falle, dass man etwas debuggen muss, insb. wenn man anderen helfen möchte, muss man schon sehen, was rausgeht und reinkommt.
Das verstehe ich nicht. Präsentiere gerne deinen Fix, dann verstehe ich vermutlich welches Problem du erkannt hast. |
Findest du in meinem letzten Commit. Und in der Funktion isDataValid() habe ich die Abfrage "_lastUpdate > 0" weil sie keinen Mehrwert bringt und ein check > 0 bei einem periodisch überlaufenden Zähler potentiell gefährlich ist mit ... ok...sehr geringem Risiko. |
Die veMpptStruct habe ich abgeändert. Namen und types angepasst. Schau dir doch mal diese Umwandlung an. |
Nice 💪
Das Feld sieht so aus: |
Du arbeitest noch weiter an diesem Thema, oder? Dann warte ich noch mit testen und mergen? |
Ja, Ich bin fast soweit. Die veShuntStruct würde ich mal so lassen, weil ich Änderungen nicht testen kann. |
Es ist nur noch der Punkt mit dem Rollover offen. Wir benötigen noch eine Lösung für den Fall das Daten empfangen wurden, ab einem Zeitpunkt X nicht mehr und dann nach ca 50 Tagen denn der Zeitpunkt X wieder erreicht wird (Rollover) die Daten für 10 Sekunden gültig werden. Ich habe 2 Lösungsvorschläge:
Beides sollte zuverlässig das Problem Lösen. Was hältst du davon? |
Ich dachte schon an 1. und habs auch angefangen als pseudo-code vorzuschlagen und dann wieder verworfen.
Allerdings sind dann die Daten echt weg, statt dass die zuletzt eingegangenen vorgehalten werden. Dabei bin ich mir unsicher, ob das zu Problemen irgendwo führt oder das Verhalten ändert, sodass wir für Leute irgendwas kaputt machen. Daher dachte ich, es ist akzeptabel, dass dieser Glitch passieren kann nach 50 Tagen. Ich würd's daher einfach ignorieren. Du? |
Stimme dir zu. Solange wir keinen wirklichen Vorteil mit einer Codeänderung sehen sollten wir es so lassen und den Glitch akzeptieren. Ich ändere das gleich noch ab und setze den PR auf review. |
Ok, von meiner Seite kannst du die Änderungen mergen. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Es sind nur noch Kleinigkeiten übrig. Falls du die nicht schaffst bis Sonntagmittag und dich auch nicht wehrst, dann übernehme ich das gerne.
Ja, mach ich.
Ich bin da mutiger und würde das übernehmen, auch wenn ich keinen SmartShunt habe. Die sind leider recht teuer, zu teuer um für Tests anzuschaffen. |
Gerade nochmal etwas beobachtet, bei current verändern sich nur die letzte Stelle und nach Komma. |
Hallo @PhilJaro , Die Werte werden im 1 Sekunden Rhythmus abgefragt. |
Bzgl. des current Problems ist mir nun noch folgendes aufgefallen, sobald die Werte Negativ werden, sprich die Batterie entladen wird stehen dort wieder die 4… Millionen, deshalb ging es bei mir heute morgen wieder, da ab dort wieder mehr geladen als entladen wurde. Sieht also so aus als könne die Änderung nicht mit negativen Werten umgehen. @SW-Niko Hier mal ein 2 Sekunden Ausschnitt: [VE.Direct MPPT 14/27] serial input (363 Bytes): |
Hallo @PhilJaro, |
Hallo @PhilJaro, PS: Die "Unhandled Hex Async Response" sind ganz normal. Die Victron Geräte tauschen jede Menge Daten untereinander aus und die bekommen wir nun auch mit. Wir werten sie nur nicht aus. |
@SW-Niko Sehr gut, jetzt werden die Negativen Werte richtig angezeigt, somit sollte alles passen. |
* process "IL", "AR" and "MON" * discard "BMV" and (unsolicited) History Data * simplify isDataValid() * veMpptStruct, veStruct: new, verbose variable names, including units, and replace floats (save values with original integer precision) * comment on rollover situation in isDataValid()
Danke @schlimmchen und @PhilJaro, |
Ja, mach das gerne, genau diesen Gedanken hatte ich auch schon.
Beides dann absolut und relativ ausgeben. @PhilJaro Da fällt mir noch was ein in diesem Zusammenhang, das ich dich fragen wollte: Wie hast du denn den Victron angeschlossen an den ESP32? Direkt? Mit Level-Shifter? Mit ADUM1201? Es ist nämlich so, dass das direkte Anschließen beim Senden definitiv zu mehr oder zumindest mit höherer Wahrschenlichkeit zu Problemem führt, als beim Empfangen. Denn der ESP32 kommt ja nur auf 3V3 am TX Pin, und das muss der Victron nicht notwendiger Weise bzw. nicht zuverlässig als High erkennen, weil er mit 5V arbeitet. |
Neuer Pull-Request, bitte. |
Rebased onto helgeerbe/development and squashed commits. |
Ich habe den SmartShunt und auch SmartSolar nicht direkt sondern jeweils über den ADUM1201 angeschlossen, die Problematik ohne ist mir schon bewusst. Danke ebenfalls für euere Arbeit ;) |
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. |