Feature: Victron solarcharger: set battery charge-current-limit #1658
Conversation
|
Bitte stelle einen separaten pull request für den fix von #1657. |
|
Nein, tue das nicht, ich hab begonnen diese Funktion umzuschreiben. Die ist mir zu verwurstelt -- meiner Meinung nach der Grund, dass der Bug überhaupt eingebaut wurde. Bitte nichts tun in Bezug auf #1657. Ansonsten hat Andreas aber recht und ich hätte das auch bemängelt. Issues zusammenwerfen ist Mist. |
|
Ok, wenn du diese Funktion bereits umschreibst, dann ist meine neue Methode getChargeCurrentLimit() vermutlich ebenfalls betroffen, da die sich die eng an getDischargeCurrentLimit() anlehnt - dabei bin ich dann über die falsche Logik gestolpert. Sagt mir einfach, wie ihr das braucht .... Einen Fork zu machen und daraus einen Pull-Request zu starten war einfach, aber wie läuft das, wenn ich jetzt noch einen Pull-Request starten will? Muss ich dazu einen Branch vom Master aufmachen mit den Änderungen und daraus dann einen Pull-Request? |
Genau so. Aber bitte von |
AndreasBoehm
force-pushed
the
development
branch
from
6f30034 to
a1d8c78
Compare
AndreasBoehm
changed the title
AndreasBoehm
force-pushed
the
development
branch
2 times, most recently
from
6fb932e to
6ed0a14
Compare
AndreasBoehm
changed the title
AndreasBoehm
force-pushed
the
development
branch
from
6ed0a14 to
ab8be13
Compare
|
Ich hab den Code gefunden den Victron nutzt um das charge current limit an die Laderegler zu verteilen als ich eigentlich was anderes gesucht habe: https://github.com/victronenergy/dbus-systemcalc-py/blob/741c38469ba804894398e700e335ec78654d2eed/delegates/dvcc.py#L587 Ich hab in deinen Code noch nicht reingeschaut aber vielleicht kannst du das ja brauchen. |
Erledigt - der praktische Test steht aber noch aus. |
Danke - sehr interessant. Ich denke es wird aber erstmal bei meinem Code bleiben, da der einen Vorteil hat: er funktioniert auch sinnvoll, wenn weitere Victron-Laderegler im System eingebunden sind und NICHT mit OpenDTU verbunden sind (z.B. weil mehr als 3 Stück verwendet werden). Sofern alle Regler als Smart-Network (per Bluetooth) zusammengeschlossen sind, wird dabei die Gesamt-Leistung durch OpenDTU erfaßt und mein Algorithmus zieht dann die Lade-Leistung der nicht direkt angeschlossenen Regler ab und verteilt nur noch den Rest auf die, die direkt angeschlossen sind. Es werden somit auch in einem solchen Fall noch die von OpenDTU steuerbaren Regler mit einem sinnvollen Limit versorgt. BTW: dabei ist mir gerade aufgefallen, dass OpenDTU onBattery bei der Solarladeregler Gesamtleistung nur die Summe der MPPT-Leistungen anzeigt und per MQTT reported, nicht aber die eventuell höhere Leistung, die das Victron VE.Smart Network als Gesamtleistung meldet - kein Beinbruch, aber unschön wenn man nicht MPPT an OpenDTU anschließen kann. Eventuell ist das dann auch beim Full-Solar-Passthrough ein Problem? |
m.M.n. ist die Anzeige der MPPT Ausgangsleistung, also die Leistung die im DC Bus für Wechselrichter oder zum laden der Batterie verfügbar ist interessanter als die Leistung am MPPT Eingang anliegt. Im Falle eins Setups bei dem nicht alle MPPTs mit der DTU verbunden sind, hast du aber absolut recht.
VE.Smart Network Leistung ist als MQTT topic verfügbar
Für (Full)-Solar-Passthrough ist das tatsächlich ein Problem. Bug #1793 |
Da hast du mich falsch verstanden. Ich habe nicht gefordert das du den Code 1:1 so übernimmst, das war nur als Inspiration. |
Mein Algorithmus orientiert sich an der tatsächlichen Leistungsabgabe, d.h. wenn ein MPPT doppelt soviel Leistung wie ein anderer abgibt, dann bekommt er auch das doppelte Limit. Nur weil ein MPPT potentiell ein potenteres Modell ist (z.B. 150/45 statt 100/30), muss er noch lange nicht tatsächlich mehr Leistung bringen. Ich habe z.B. 3 gleiche 150/45 im Einsatz, wovon einer mit 6 Panels belegt ist und ein anderer nur mit 3, weil das mit Verschattung etc. die sinnvollste Lösung war. Dafür aber ein kleineres Modell (150/35) zu wählen, machte nur einen vernachlässigbaren Unterschied im Preis aus, verbaut aber potentielle Anlagenerweiterungen. Noch kleiner und günstiger geht erst recht nicht, da dann keine 48V-Akku-Ladung mehr angeboten wird. Kurz: rein am Modell, bzw. dessen maximaler Leistung, kann man meiner Meinung nach die Verteilung höchstens abschätzen, aber nicht mehr. |
There was a problem hiding this comment.
Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
include/solarcharger/victron/Provider.h:21
- [nitpick] The method signature includes an 'act_charge_current' parameter, but its intended usage is not clear from the diff. Please verify that the implementation uses this parameter as needed or document its purpose.
void setChargeLimit( float limit, float act_charge_current) final;
lib/VeDirectFrameHandler/VeDirectMpptController.h:79
- [nitpick] There are two ChargeCurrentLimit entries in the hex queue (one for GET and one for SET). A clarifying comment could help explain the rationale behind this duplication to prevent confusion.
VeDirectHexRegister::ChargeCurrentLimit, false, 4, 0, _no_data, 0,
@schlimmchen hat das Problem in #1817 gelöst |
Das macht definitiv Sinn!
100/20 wäre der kleinste den es mit 48V Output gibt.
Genau das meinte ich. Das sollte sich auslesen lassen wenn ich den Code von Victron richtig interpretiert habe. Angenommen ich habe einen 150/35 und einen 250/70. Jetzt stellen wir 40A für den 35er ein und 40A für den 70er -> 5A 'verloren' weil der 35er das gar nicht liefern kann. |
Ja, das wirkt so im ersten Moment, wenn man das Ladelimit verfolgt, aber tatsächlich liefert der 35er dann die 35A und der 70er dann 40A, womit sich das Verhältnis ändert und das Limit beim 70er automatisch im nächsten Durchlauf erhöht wird, bis das Limit dem tatsächlichen Verhältnis Rechnung trägt. |
|
Ok - im vorhandenen Code von OpenDTU-onBattery scheint schon mal keine Angabe des Maximums enthalten zu sein. Aus dem Python-Code von Victron habe ich auch noch nicht herauslesen können, ob diese Angabe einfach aus dem Modellnamen extrahiert wird oder per HEX-Protokoll ermittelt wird. In der Spezifikation des HEX-Protokolls widerrum finde ich keinen Wert, der sich für mich eindeutig als device-spezfisches Maximum ergibt. Allenfalls 0xEDF0 Battery maximum current klingt noch geeignet - das scheint ohnehin bei der Anwendung des Charge Limits angewendet zuu werden, wenn es kleiner als das gesetzte Limit ist. Ich weis aber nicht, ob da im Auslieferungszustand was sinnvolles drinsteht, oder eher ein unlimitierter Default-Wert - der Wert ist nicht konstant, sondern non-volatile beschreibar, also eher ein Wert der konfiguriert wird, als eine device-spezifische Konstante. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Ich habe heute mal probeweise den HEX-Parameter "Battery Maximum Current" mit in diem Liste der zyklisch gelesenen Kommandos aufgenommen und tatsächlich - dieser ist per Default auf das Maximum des MPPT-Reglers gesetzt. Per Victron-APP (und VE.Direct) ist dieser auch nur bis zum maximal möglichen Strom des Regler konfigurierbar. Der Wert erscheint mir daher als sinnvoll für die Limitierung des Charge-Limits pro Controller. Ich habe meine Verteilungslogik entsprechen angepasst - jedes Limit wird darauf beschränkt und der Rest dann an die übrigen Controller verteilt. Funktioniert bei mir auch soweit. |
|
Ich wäre daran interessiert, dass dieses Feature eingeführt wird. Leider vestehe ich nicht, ob das vorgesehen ist, uns falls ja, wann. Könnte mir das mal jemand kurz erklären (ggf mit dem Hinweis wo im Verlauf der Diskussion ich was hätte erkennen können)? Sorry für die wahrscheinlich dumme Frage, aber ich habe keine Idee wie ich sonst zue einer Antwort kommen könnte. |
|
@gonzo8855 Grundsätzlich spricht nichts dagegen das dieses Feature gemerged wird und in einem Release landet. Wir hatten einfach noch keine Zeit im Detail drauf zu schauen. Einen Zeitpunkt kann ich dir nicht nennen, das vermeiden wir, ist ja alles Freizeit die wir hier reinstecken und es kann immer was dazwischen kommen. |
|
This pull-request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Hallo, |
Ja, läuft bei mir seit einigen Monaten erfolgreich. Allerdings habe ich wohl bei den letzten Syncs was falsch gemacht - mein PR compiliert nicht mehr und hat bei vielen Dateien Unterschiede zum Code von Hoylabs-Development, obwohl mir Github sagt, ich wäre auf dem aktuellen Stand. @AndreasBoehm: Kannst du mir da helfen den PR zu aktualisieren bzw. was muss ich tun? |
|
@hhoebel Klar, kein Problem. Ich werden deinen PR die Tage mal rebasen. Für ein review wird die Zeit aber vermutlich noch nicht reichen, sorry. |
DANKE! Ich will nur den PR aktuell halten, das reicht mir erstmal. |
Co-authored-by: Copilot <[email protected]>
AndreasBoehm
force-pushed
the
development
branch
from
aa90178 to
03edd6c
Compare
|
Hab den PR rebased, hab nur leider keinen ESP zur Hand um zu testen ob alles funktioniert, es kompilier zumindest. |
Build ArtifactsFirmware built from this pull request's code:
Notice
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds full support for propagating battery charge-current limits to Victron MPPT controllers, including new config options, stats tracking, and communication commands.
- Extend
Stats,Provider, andControllerto record, publish, and apply charge current limits. - Introduce configuration parameters (min/max limits and SoC/voltage thresholds) with serialization and defaults.
- Enhance
VeDirectMpptControllerto send and receive charge-current-limit commands alongside existing registers.
Reviewed Changes
Copilot reviewed 20 out of 31 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/battery/pytes/Provider.cpp | Switch raw field write to setChargeCurrentLimit(...) |
| src/battery/pylontech/Stats.cpp | Remove always-published chargeCurrentLimitation to defer to global |
| src/battery/Stats.cpp | Add conditional live-view & MQTT publish of chargeCurrentLimit |
| src/battery/Controller.cpp | Implement getChargeCurrentLimit() with config and BMS thresholds |
| src/Configuration.cpp | Serialize/deserialize new charge limit settings |
| include/Configuration.h | Add new members to BatteryConfig |
| include/defaults.h | Define defaults for new charge limit config values |
| include/battery/Stats.h | Add getChargeCurrentLimit and tracking of last update timestamp |
| include/solarcharger/Provider.h & MQTT Provider | Add setChargeLimit(...) stub for all SolarChargers::Provider |
| lib/VeDirectFrameHandler/VeDirectMpptController.h/.cpp | Add ChargeCurrentLimit and BatteryMaximumCurrent regs, queue send |
| lib/VeDirectFrameHandler/VeDirectData.h | Extend veMpptStruct and VeDirectHexRegister for new registers |
Comments suppressed due to low confidence (2)
src/battery/Controller.cpp:154
- [nitpick] Add a brief doc comment summarizing the purpose, inputs, and behavior of
getChargeCurrentLimit()for future maintainers.
float Controller::getChargeCurrentLimit()
src/battery/Controller.cpp:154
- This method has several branches (min/max thresholds, BMS-derived limits); consider adding unit tests to cover all key scenarios.
float Controller::getChargeCurrentLimit()
| @@ -62,12 +65,17 @@ class VeDirectMpptController : public VeDirectFrameHandler<veMpptStruct> { | |||
|
|
|||
| uint32_t _sendTimeout = 0; // timeout until we send the next command from the queue | |||
| size_t _sendQueueNr = 0; // actual queue position; | |||
| uint32_t _chargeLimit = 0xFFFF; // limit MPPT to this limit (in 0.1A), default: 0xFFFF (=full current) | |||
| uint32_t _no_data = 0; // dummy-value | |||
|
|
|||
| // for slow changing values we use a send time period of 4 sec | |||
| #define HIGH_PRIO_COMMAND 1 | |||
There was a problem hiding this comment.
[nitpick] Avoid defining macros inside class headers; consider replacing HIGH_PRIO_COMMAND with a static constexpr uint8_t member or an enum.
| #define HIGH_PRIO_COMMAND 1 | |
| static constexpr uint8_t HIGH_PRIO_COMMAND = 1; |
Copilot uses AI. Check for mistakes.
| @@ -23,6 +23,7 @@ class Controller { | |||
| mutable std::mutex _mutex; | |||
| std::unique_ptr<Provider> _upProvider = nullptr; | |||
| bool _forcePublishSensors = false; | |||
| float _actChargeLimit = 0.0f; | |||
There was a problem hiding this comment.
[nitpick] Field _actChargeLimit is declared but never used in this class; remove it or integrate it into the charge-limit workflow.
| float _actChargeLimit = 0.0f; |
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
Scheint erstmal gut zu funktionieren auf meinem System - Danke Dir! |
| target["charge_current_limit_below_soc"] = config.Battery.ChargeCurrentLimitBelowSoc; | ||
| target["charge_current_limit_below_voltage"] = config.Battery.ChargeCurrentLimitBelowVoltage; |
There was a problem hiding this comment.
I think that we should change this to current limit above XYZ because the charge current should get reduced/limited when the battery is getting fuller, not when its getting emptier.
There was a problem hiding this comment.
I introduced those settings:
1.) "charge_current_limit_below_xxx": the limit for the charge current when the battery is empty - most batteries don't allow full charge current in this state. You'll need the min, if the BMS doesn't report a limit (e.g. if you don't have a limit from a BMS, but the SOC from an Smart Sense).
2.) "max_charge_current_limit": this limits the charge current when the BMS doesn't report a limit (e.g. no BMS connected) or you want to limit to a lower limit (e.g. an additional charger is attached)
3. )"min_charge_current_limit": this gurantees that the limit doesn't ever goes down to zero. My BMS reports a limit of 0A whenever a cell overvoltage occurs, but stops balancing as soon as the charge current disappears. To continue balancing the charge current has to remain at a low level (~2A).
None of these settings is special for Victron, but for special use-cases.
| target["max_charge_current_limit"] = config.Battery.MaxChargeCurrentLimit; | ||
| target["min_charge_current_limit"] = config.Battery.MinChargeCurrentLimit; |
There was a problem hiding this comment.
I understand why you added min and max but the min value is a victron or solarcharger specific setting because you don't want to set current limit to 0. But if we focus on the battery alone, do i want to charge my battery with 3 A even if the BMS reports 0 A? Another thing to keep in mind is that ideally a grid charger (only Huawei R48xx right now) also works based on the charge current limit and would keep charging with the minimum current limit.
If you agree that this is a victron/solarcharger only setting and not actually related to charging the battery but how much power the victrons should provide no matter what the BMS tells us, we need to either agree on a good hardcoded default value or create a settings item in the victron config.
what do you think?
There was a problem hiding this comment.
Here my thoughts:
My battery BMS tuns "Max Charge Current" to 0A if e. g. a cell over voltage warning flag occures. I would like to be able to set the victron to exaxt this value then. When the warning flag disappears, the "Max Charge Current" goes back to 94,5A. This is much more than the Victron could deliver.
I don't know what a victron that could deliver 20A would do with an value above that.
So I would think that such a configuration is an victron only configuration that should be activatable and disactivatable and Victron specific max / min values should be configuratable. (I would adjust it in my case to 20A / 0A).
There was a problem hiding this comment.
Victron max current does not need to be configurable within OpenDTU. If the battery allows 90 A but the victron can only do max 30 A, we will set it to 30 A. If you want to lower the max value of the victron you should do so in the victron mppt itself using the victron app.
Talking about this, i wonder why we need the min setting at all? If a inverter connected to the DTU produces power the current drawn by the inverter is part of the current limit send to the victrons and this means that even if the BMS reports 0 A we would still allow the victrons to provide the required power to feed the inverter without taking power from the batteries.
There was a problem hiding this comment.
Ok, now I start to understand the philosophy:
The values for the Vicctron max current are the desired Battery charging current plus the solar passthrough current. This means even if battery current demand is 0A the victron will be adjusted to deliver the solar passthrough current.
This means the Victron max current limiter will only apply if all inverters cannot digest the not limitetd Victron current and the battery demands a lower charging current.
Then you are right and we pobably do not need the min setting.
The max setting would apply if there is no desired Battery charging current delivered by the battery. (Does the Max setting in the Victron App limit the Value transmitted by DTU on Battery or will it just be overwitten?)
There was a problem hiding this comment.
Exactly.
The max current set in the victron app will be respected by the DTU, we will never request more current then what was set by the victron app. Actually these are two different fields in the victron mppt, and only because of that we are able to always respect that setting. By default this setting is the hardware defined max charge current e.g. 35A for the MPPT 150/35.
Ich verwende 3 Victron SmartSolar, die mir bis zu 130A liefern. Wenn meine Batterien relativ leer sind und wenn Bedarf zum Balancieren besteht, meldet die Batterie aber einen deutlich niedrigeren gewünschten Ladestrom.
Ich habe daher die Victron-Implementierung erweitert, so dass auch ein Ladestrom-Limit an die MPPT gesendet wird und entsprechend eine Methode eingeführt, die das Durchreichen des Limits bis zu den Ladereglern erlaubt.
Das Lade-Limit wird dabei entsprechend der aktuellen Stromabgabe der Victrons prozentual auf diese ausgeteilt, so dass auch bei begrenztem Sonnenschein die Limits vernünftig verteilt werden und nicht die Leistungsabgabe unnötig einschränken.
In der Batterie-Konfiguration habe ich Einstellungen für ein minimales Limit (das wird immer verwendet, sobald der eingestellte SoC oder Voltage überschritten wird und die Batterie ein kleineres Limit fordert. Hintergrund: Meine Batterie wechselt auf ein Lade-Limit von 0A sobald der SoC 100% erreicht. Ich möchte dann aber noch etwas Strom erlauben, um die Spannung der Batterie zu halten zwecks Balancing.
Weiterhin gibt es maximales Limit, das bis zum einstellbaren SoC /Voltage-Wert verwendet wird, falls die Batterie kein Lade-Limit meldet.
Die bei den jeweiligen Victrons gesetzten Lade-Limits erscheinen in der Live-View.
Das Lade-Limit wird zudem um den aktuellen aufgenommen Strom der an der Batterie hängenden Inverter erhöht, damit keine unnötige Bregrenzung durch Entnahme des Inverters erfolgt.
Die Implementierung ist mit 3 SmartSolar und 3 Akkus (Pylontech-kompatibel) getestet.
Siehe auch
#1276
und
#470