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

Implement VE.Direct hex communication #739

Closed
wants to merge 0 commits into from

Conversation

SW-Niko
Copy link

@SW-Niko SW-Niko commented Mar 11, 2024

Ganz grob zusammengefasst habe ich folgendes implementiert.

Den VictronFrameHandler habe ich erweitert, damit alle von ihm abgeleiteten Klassen Zugriff auf den Hex-Mode bekommen.
• sendHexCommand(), Sendet Kommandos über die VE.Direct Schnittstelle
• hexDataHandler(), In abgeleiteten Klassen muss die Methode überschrieben werden um die Hex Nachrichten zu verarbeiten.
• Einige kleine Hilfsfunktionen zum Umwandeln der Daten in das Hex-Format, berechnen der Checksumme, usw.

Bitte prüfen und konstruktives Feedback geben. (Ich programmiere noch nicht so lange und deshalb ist dieser Code wahrscheinlich etwas holprig umgesetzt)

Ich schiebe dann als nächstes das Auslesen der Gesamtleistung von dem MPPT nach..... (Noch beim testen)

@schlimmchen
Copy link
Member

Okay, cool, sieht ganz okay aus.

Ich schiebe dann als nächstes das Auslesen der Gesamtleistung von dem MPPT nach..... (Noch beim testen)

Brauchst du dabei Hilfe? Das scheint mir essentiell zu sein, denn sonst tut der neue Code noch nichts, korrekt?

Was ist dafür zu tun? Man muss regelmäßig eine entsprechende Anfrage abschicken, und die Antwort wird dann automatisch verarbeitet?

@schlimmchen schlimmchen changed the title Add of VE.Direct hex communication Implement VE.Direct hex communication Mar 13, 2024
@SW-Niko
Copy link
Author

SW-Niko commented Mar 13, 2024

Brauchst du dabei Hilfe? Das scheint mir essentiell zu sein, denn sonst tut der neue Code noch nichts, korrekt?

Ja genau, dieser "Pull request" ist nur die Basis. Ermöglicht also das "Anwenden" des Hex Protokolls.
Ich bin jetzt beim ausprobieren, was damit alles möglich ist. Und wo es Probleme gibt.
Kurz gefasst kann man sagen: Man kommt an alles ran was der über VE.Direkt verbundene MPPT bietet, aber man kommt nicht an alle Daten der anderen MPPTs ran, die über das VE.Smart Network (Bluetooth) verbunden sind.
grafik

Hilfe benötige ich an allen Ecken und Enden. Nein, so schlimm ist es nicht.
Ich melde mich einfach, wenn ich nicht mehr weiterkomme und ansonsten bin ich für jede Anregung zum besseren programmieren dankbar.

Was ist dafür zu tun? Man muss regelmäßig eine entsprechende Anfrage abschicken, und die Antwort wird dann automatisch verarbeitet?

Genau so läuft das. Ich schiebe in den nächsten Tagen einen weiteren "Pull request" zum VeDirectMpptController nach.
Darin sieht man wie das mit den Hex Nachrichten funktioniert. Als Resultat stehen dann die Werte in der MPPT-Klasse mit Zeitstempel zur Weiterverarbeitung zur Verfügung.

@SW-Niko
Copy link
Author

SW-Niko commented Mar 14, 2024

Ok das ging schon mal schief. Ich wollte eigentlich einen neuen "Pull reqeust" aufmachen.

In der MPPT class werden jetzt drei Werte (TotalDCPower, MPPT Temp und Batterie Temp) über die Hex Nachrichten ausgelesen und zur weiteren Verwendung zur Verfügung gestellt.

@schlimmchen Wie geht das jetzt weiter? Benötigst du weitere Tests oder Unterlagen?
Ich würde es begrüßen wenn die Daten erst mal nur Angezeigt werden und in einem zweiten Schritt für die die Steuerung verwendet wird. Soweit bin ich aber noch nicht. Habe mit der Webpage noch gar nichts gemacht und den DPL durchblicke ich auch (noch) nicht. Sorry.

veMPPTExStruct const *getExData() const { return &_ExData; }

protected:
virtual void hexDataHandler(VeDirectHexData const &data) override;
Copy link
Member

Choose a reason for hiding this comment

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

override, sehr gut, warum nicht final? 😉

Copy link
Author

Choose a reason for hiding this comment

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

Ok, verstanden. Genau das brauch ich noch. Ein kurzer Hinweis reicht.

@@ -68,10 +68,28 @@ class VeDirectMpptController : public VeDirectFrameHandler {
using spData_t = std::shared_ptr<veMpptStruct const>;
spData_t getData() const { return _spData; }

void loop(); // main loop to read ve.direct data
Copy link
Member

Choose a reason for hiding this comment

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

warum ist das nicht override bzw. nicht final?

Copy link
Author

Choose a reason for hiding this comment

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

Ok

uint32_t value; // value from register
char text[VE_MAX_HEX_LEN]; // text/string response
};

class VeDirectFrameHandler {
public:
VeDirectFrameHandler();
virtual void init(int8_t rx, int8_t tx, Print* msgOut, bool verboseLogging, uint16_t hwSerialPort);
void loop(); // main loop to read ve.direct data
Copy link
Member

Choose a reason for hiding this comment

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

Sollte loop() nicht virtual sein, weil du versuchst die Methode in VeDirectMpptController zu überschreiben?

Copy link
Author

Choose a reason for hiding this comment

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

Ok, Verstanden warum das besser ist.

@schlimmchen
Copy link
Member

Ok das ging schon mal schief. Ich wollte eigentlich einen neuen "Pull reqeust" aufmachen.

Das mag sein, aber es ist so gewollt, dass du an diesem Pull-Request weiterarbeitest, bis er alles enthält, sodass du sagst "so finde ich könnte er in ein Release". Bis dahin sollten wir ihn als Draft (Entwurf) einstufen, aber sonst ist das genau, wie das laufen soll. Du fügst solange commits hinzu, bis du zufrieden bist.

Sorry.

Du musst dich für nichts entschuldigen, mach gerne einfach weiter.

Was ist der nächste Schritt? Die Daten, falls sie denn abgefragt wurden und die Antwort verarbeitet wurde, zur Web Application transportieren?

@schlimmchen schlimmchen marked this pull request as draft March 14, 2024 18:50
@SW-Niko
Copy link
Author

SW-Niko commented Mar 15, 2024

Was ist der nächste Schritt? Die Daten, falls sie denn abgefragt wurden und die Antwort verarbeitet wurde, zur Web Application transportieren?

Genau, Wenn du das bei Gelegenheit machen könntest.

@SW-Niko SW-Niko marked this pull request as ready for review March 15, 2024 10:00
@SW-Niko
Copy link
Author

SW-Niko commented Mar 17, 2024

Hallo @schlimmchen ,
ich habe die Diskussion im PR #756 verfolgt. Soll ich die 3 double Werte entfernen und auf 10mW und 10m°C abändern und so lassen wie sie über die Schnittstelle kommen?

grafik

@schlimmchen
Copy link
Member

Ich fänds sinnvoll, wenn diese Werte mit 10 multipliziert würden, und dann als mV, mW, und m°C gespeichert würden, als Integer. Die Temperatur muss int32_t (int16_t reicht bei m°C nicht, dann passt nur ca. +/-32°C rein) sein, die anderen zwei uint32_t.

@SW-Niko
Copy link
Author

SW-Niko commented Mar 17, 2024

Kein Ding, mach ich gleich. Ja richtig, dann müssen wir für alle 3 Werte 32bit nehmen
Total DC input power: Nur positive Werte. Mit uint16_t komm ich dann nur bis 65W. Das reicht nicht ... also uint32_t
MPPT Temperatur: Auch negative Werte. Mit int16_t sind es dann nur ±32°C. Reicht auch nicht ... also int32_t
Batterie Temperatur: Auch negative Werte. Mit int16_t sind es dann nur ±32°C. Reicht auch nicht ... also int32_t

PS: Falls das untergegangen ist. Die Spannung vom "Smart Battery Sense" wird von den MPPTs automatisch übernommen und wir lesen diesen Wert über die Textnachrichten rein (Variable V). Das ist super praktisch ... Für 32€ bekommst du eine Spannungs- Messung an den Batteriepolen und die DTU nimmt das als Regelgröße wenn man kein auslesbares BMS hat. Muss ich bei Gelegenheit ins Wiki schreiben.

@philippsandhaus
Copy link

Hallo @SW-Niko, die Erweiterung finde ich super! Ich hatte vor ein paar Monaten auch schon einmal daran gearbeitet #479, hab es dann aber mangels Zeit und PV-armen Winter liegen lassen. Vielleicht kannst Du ja etwas von dem Code gebrauchen. Ich hatte damit begonnen, weil ich Unterstützung für meinen Victron AC-Lader hinzufügen wollte, der über das HEX-Protokoll arbeitet #436 . Das Projekt habe ich vor, wieder aufzunehmen und würde mich dann jetzt auf deine Erweiterung stützen.

@SW-Niko
Copy link
Author

SW-Niko commented Mar 18, 2024

Hallo @philippsandhaus , klar mach nur. Falls es Probleme mit den Hex Nachrichten gibt dann einfach bei mir melden.
Ich habe aber nur die MPPT hex Nachrichten implementiert. Falls der AC-Lader zusätzliche Befehle benötigt dann kann ich das auch einbinden. Nur Testen kann ich nicht, weil ich keinen AC-Lader habe.

@philippsandhaus
Copy link

Super. Mein Plan wäre erst einmal, das über eine eigene Klasse zu realisieren, die von VEDirectFrameHandler ableitet und hier die spezifischen Befehle einzubinden (ich möchte die Ladeleistung dynamisch anpassen, je nach verfügbarem Überberschuss). In einem zweiten Schritt könnten wir ja schauen, ob es Funktionen gibt, die den MPPTController und den ACLader interessant und die man dann ich eine gemeinsame Klasse zusammenziehen könnte. Super ist aber schon einmal deine grundsätzliche Implementierung der HEX-Kommunikation.

@SW-Niko
Copy link
Author

SW-Niko commented Mar 19, 2024

Hallo @schlimmchen,
ich glaube der Konflikt kommt vom PR #706 .
Das kann ich vermutlich nicht lösen. Oder kannst du mir einen Tipp geben?

@SW-Niko
Copy link
Author

SW-Niko commented Mar 21, 2024

ohje.... Habe Mist gebaut. Ich versuch es nochmal.

@SW-Niko SW-Niko reopened this Mar 21, 2024
@SW-Niko SW-Niko requested a review from schlimmchen March 29, 2024 14:52
@SW-Niko
Copy link
Author

SW-Niko commented Apr 2, 2024

Hallo @philippsandhaus ,
Nur zur Info....
Ich muss in meinem Branch-Handling ein paar Korrekturen vornehmen und dadurch wird der PR vermutlich geschlossen.
Keine Bange. Den PR stell ich wieder online wenn ich alles angepasst habe.

@SW-Niko SW-Niko marked this pull request as draft April 2, 2024 14:19
@SW-Niko SW-Niko closed this Apr 2, 2024
@schlimmchen
Copy link
Member

Niko, du musst insbesondere bei deinem nächsten Beitrag einen Branch erzeugen und nutzen, der nur für die gewünschte Anpassung gedacht ist (statt deinem development Branch, an dem du dann "wild" hin- und her-merge'st).

Was das Öffnen eines neuen PR angeht: Darfst du gerne machen, aber Arbeit musst du keine mehr investieren. Deine Arbeit habe ich übernommen (Anpassungen waren nötig nach dem Rebase auf den aktuellen development Branch) und erweitert und plane das bald zu mergen. Der Code läuft insb. seit einem Tag auf meinem Produktiv-System 👍

@SW-Niko
Copy link
Author

SW-Niko commented Apr 2, 2024

Prima. 👍
Meinen Fehler im "Git-Workflow" konnte ich korrigieren. Das solle bei meinem nächsten PR viel besser laufen.
Vielen Dank für deine Mühe mit mir.

@schlimmchen
Copy link
Member

Vielen Dank für deine Mühe mit mir.

Lieb, dass du dankbar bist, aber der Dank ist ganz meinerseits: Vielen Dank für die gute Arbeit. 💪

Copy link

github-actions bot commented May 3, 2024

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 May 3, 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