kostal_plenticore #2440
kostal_plenticore #2440
Conversation
|
Ich denke ein Beispiel ist aussagekräftiger, als eine lange Beschreibung, deswegen habe ich als Beispiel das HTTP-Modul in #2441 auf Was den tcp_client betrifft kannst du entweder den auch im Konstruktor übergeben oder in der Für das |
| component_updater=MultiComponentUpdater( | ||
| lambda update_components: update_components(components_todo, tcp_client)), |
There was a problem hiding this comment.
update_components ist keine Funktion, sondern die Komponenten. Also ein Iterable mit KostalPlenticoreBat oder KostalPlenticoreInverter oder KostalPlenticoreCounter. Diese haben praktischerweise alle eine Methode update(self, reader: Callable[[int, ModbusDataType], Any]). Die musst du aufrufen. So:
def update_components(components: Iterable[Union[KostalPlenticoreBat, KostalPlenticoreCounter, KostalPlenticoreInverter]]):
for component in components:
component.update(reader)
MultiComponentUpdater(update_components)There was a problem hiding this comment.
Vor dem Aufruf der for-Schleife würde ich gerne noch den Kontextmanager für den tcp-Client setzen, damit die Verbindung nur einmal für alle Komponenten geöffnet wird. Dazu brauche ich aber den tcp_client, so wie hier:
https://github.com/snaptec/openWB/blob/5cc5eebb5ec631bbb32aeedc6969b4dc24c2408a/packages/modules/kostal_plenticore/device.py#L23
There was a problem hiding this comment.
Vor dem Aufruf der for-Schleife würde ich gerne noch den Kontextmanager für den tcp-Client setzen, damit die Verbindung nur einmal für alle Komponenten geöffnet wird.
Ich hatte in Gedanken gedacht du machst die Verbindung wahrscheinlich einmal für die Lebenszeit vom Device auf. Nur einmal pro Durchlauf für alle Komponenten ist natürlich auch eine Möglichkeit. Vielleicht sogar die bessere. In dem Fall bist du mit dem Konstruktorargument nicht so gut beraten. Es wäre möglich, ich glaube aber es würde unübersichtlich. Mit dem Parameter für update wie du es vorher bereits hattest aber ganz simpel. Alles zusammengefügt zum Beispiel so:
def create_device(device_config: KostalPlenticore):
def create_bat_component(component_config: KostalPlenticoreBatSetup):
return KostalPlenticoreBat(device_config.id, component_config)
def create_counter_component(component_config: KostalPlenticoreCounterSetup):
return KostalPlenticoreCounter(device_config.id, component_config)
def update_components(
components: Iterable[Union[KostalPlenticoreBat, KostalPlenticoreCounter, KostalPlenticoreInverter]]
):
with modbus.ModbusTcpClient_(device_config.configuration.ip_address, 1502) as tcp_client:
def reader(register: int, data_type: modbus.ModbusDataType):
return tcp_client.read_holding_registers(register, data_type, unit=71, wordorder=Endian.Little)
for component in components:
component.update(reader)
return ConfigurableDevice(
device_config=device_config,
component_factory=ComponentFactoryByType(
bat=create_bat_component, counter=create_counter_component, inverter=KostalPlenticoreInverter
),
component_updater=MultiComponentUpdater(update_components),
)Im Moment hast du die Funktion update_components nicht innnerhalb von create_device und daher kein Zugriff auf den device_config. Ich emphele die create_device-Funktion möglichst kurz zu halten. Das ganze caging von Variablen ist in kleinen Funktionen überhaupt kein Problem, aber wenn die Funktion wächst wird es unübersichtlich. Da deine update_components-Funktion schon recht lang ist, würde ich dir emphelen diese nicht hier komplett unterzubringen sondern so wie jetzt auch top-level zu belassen und stattdessen einen Parameter für die device-config oder direkt die Adresse hinzuzufügen. Hier kannst du dann entweder mit einer kleinen Hilfsfunktion wie den create_x_component Funktionen an die oben delegieren oder mit functools.partial arbeiten.
Übrigens Randnotiz: Das hier:
def reader(register: int, data_type: modbus.ModbusDataType):
return tcp_client.read_holding_registers(register, data_type, unit=71, wordorder=Endian.Little)Ist gleichbedeutend mit:
reader = functools.partial(tcp_client.read_holding_registers, unit=71, wordorder=Endian.Little)Was man da nimmt ist natürlich Geschmackssache. Beides hat den selben Effekt.
Ich dachte, das erledigt der Kontextmanager. |
Du kannst den Kontextmanager nicht über mehrere Durchläufe der Regelschleife aufrecht halten. Daher dachte ich du brauchst was auf Device-Ebene. Wenn du allerdings die Verbindung nur für einen Durchlauf der Regelschleife aufrecht erhalten möchtest hast du natürlich recht. Der Kontextmanager regelt das für dich. |
LKuemmel
force-pushed
the
kostal_plenticore
branch
from
d989858 to
1fe8a95
Compare
| for component in components: | ||
| if isinstance(component, KostalPlenticoreBat): | ||
| bat_state = component.update() | ||
| else: | ||
| bat_state = None |
There was a problem hiding this comment.
Ich vermute du möchtest, dass wenn keine Batterie konfiguriert ist, dass bat_state zu None wird. Das hier oben setzt bat_state jedoch nur auf None, wenn garkeine Komponente konfiguriert ist. Du willst wahrscheinlich eher:
bat_state = None # type: Optional[BatState]
for component in components:
if isinstance(component, KostalPlenticoreBat):
bat_state = component.update()
breakoder alternativ:
bat_state = next((component.update() for component in components if isinstance(component, KostalPlenticoreBat)), None)Diese Variante hat den Charme, dass die type-annotation nicht nötig ist, denn sie kann automatisch bestimmt werden.
Beide Beispiele geben dir von der ersten konfigurierten Batterie das Ergebnis. Wenn es nur eine konfigurierte Batterie geben kann ist das sicher. Wenn es auch mehrere geben kann müsstest du die stattdessen aufsummieren oder irgendwas anderes was sinnvoll ist.
Da du weiter unten (in Zeile 46) nochmal die Batterie raussuchst, könnte es auch sinnvoll sein eine Referenz auf die Batterie zu behalten. Also weitere Alternative:
battery = next((component for component in components if isinstance(component, KostalPlenticoreBat)), None)
bat_state = battery.update() if battery else NoneThere was a problem hiding this comment.
Nein, es kann nur eine Batterie konfiguriert werden.
| else: | ||
| component.update() |
There was a problem hiding this comment.
In dem Fall könnte es sich auch um eine Battery handeln die bereits abgefrühstückt ist.
| ips = ipparser(ip3) | ||
| # in IP3 kann ein aufeinanderfolgende Liste enthalten sein "192.168.0.1-3" | ||
| if len(ips) > 1: | ||
| ip3 = ips[0] | ||
| ip4 = ips[1] | ||
| ip5 = ips[2] |
There was a problem hiding this comment.
ips ist eine Liste. Hier machst du aus der Liste einzelne Variablen. Die einzige Verwendung davon ist unten in Zeile 120, wo du wieder eine Liste brauchst und entsprechend aus den einzelnen Variablen wieder ein machst. Ich finde dann kannst du es gleich bei einer Liste belassen.
Wenn ip3 == "eine_adresse", dann ist len(ips) == 1, der if-Block wird komplett ignoriert und damit auch die konfigurierte ip3. Absicht?
Wenn ip3 == "192.168.178.1-2", dann ist len(ips) == 2. Der Block spring an, aber ip5 = ips[2] wirft einen Fehler.
Wenn ip3 == "192.168.178.1-5", dann ist len(ips) == 5. Der Block spring an, ein Teil der konfigurierten IP-Adressen wird stillschweigend ignoriert. Absicht?
Du unterscheidest weiter unten zwischen der ip1 und allen anderen ips. Das mag sinnvoll sein, das überblicke ich hier nicht. Unter der Annahme, dass das so bleibt... Wie wäre es mit:
additional_ips = [filter("none".__ne__, chain([ip2], ipparser(ip3)))](Unnötigerweise wird bei diesem Beispiel der filter auch auf das Ergebnis von ipparser angewandt anstatt vorher bereits auf die ip3 selbst zu schauen. Der Effekt ist aber der Selbe und es ist sehr kompakt so).
Oder wenn ip2 doch auch das gleiche Format unterstützen soll:
additional_ips = [parsed_ip for ip in [ip2, ip3] if ip != "none" for parsed_ip in ipparser(ip)]There was a problem hiding this comment.
Laut Kommentar und altem Code sind es entweder eine oder drei IPs, aber so sind wir auf der sicheren Seite.
| with tcp_client: | ||
| update(components) |
There was a problem hiding this comment.
Auf diese Weise hast du eine unsichtbare temporale Abhängigkeit bei der Reihenfolge der Aufrufe geschaffen. Das with tcp_client öffnet und schließt die Verbindung. Die Die Klassen KostalPlenticoreInverter etc. wissen davon nichts. Sie sehen die Abhängigkeit auch nicht, weil sie den reader bereits im Konstruktor bekommen haben. Nichts hält diese Klassen daran zurück bereits im Konstruktor den reader aufzurufen. Tatsächlich ist aber vorgesehen, dass der reader nur in der Methode update aufgerufen wird.
Wenn du den reader zu einem Parameter von update machst und auch die update-Methoden der einzelnen Komponenten mit diesem Parameter ausstattest und den Konstruktorparameter wieder entfernst, dann würdest du damit viel deutlicher zeigen, dass der reader nur in update verwendet werden soll und sonst nirgends.
| } | ||
|
|
||
|
|
||
| def read_legacy(component_type: str, ip1: str, ip2: str, battery: int, ip3: str) -> None: |
There was a problem hiding this comment.
Die Funktion ist recht lang. Es könnte Sinn ergeben inverter und counter in seperate Funktionen zu trennen. So ähnlich wie zum Beispiel auch beim HTTP-Modul.
|
|
||
| def update(self) -> None: | ||
| power_factor = self.__reader(150, ModbusDataType.FLOAT_32) | ||
| currents = [self.__reader(register, ModbusDataType.FLOAT_32) for register in [222, 232, 242]] |
There was a problem hiding this comment.
Warum nicht:
currents = self.__reader(222, [ModbusDataType.FLOAT_32]*3)(Und das gleiche auch unten bei powers)
There was a problem hiding this comment.
Dann werden aber doch die Register 222, 224, 226 ausgelesen und nicht 222, 232, 242?
There was a problem hiding this comment.
Stimmt. Mein Fehler. Pl0x ignore.
| openwbDebugLog ${DMOD} 2 "RET: ${ret}" | ||
| speicherleistung=$(<"${RAMDISKDIR}/speicherleistung") | ||
| openwbDebugLog ${DMOD} 1 "BattLeistung: ${speicherleistung}" | ||
| # wr_plenticore will fetch data for both inverter end counter and there is nothing left for us to do. |
There was a problem hiding this comment.
Da ist ein Tippfehler. Soll wohl and und nicht end sein. Der Satz kommt mir allerdings bekannt vor. Ist wohl ein Fehler für den ich die Vorlage geliefert habe. Ist gefixt in #2448
| for component in components: | ||
| if isinstance(component, KostalPlenticoreBat): | ||
| component.set(bat_state) |
There was a problem hiding this comment.
| for component in components: | |
| if isinstance(component, KostalPlenticoreBat): | |
| component.set(bat_state) | |
| battery.set(bat_state) |
| raw_inv_power = inverter_state.power | ||
| inverter_state.power = dc_in / (dc_in + bat_state.power) * raw_inv_power | ||
| # Speicherladung muss durch Wandlungsverluste und internen Verbrauch korrigiert werden, sonst | ||
| # wird ein falscher Hausverbrauch berechnet. Die Verluste fallen hier unter den Tisch. | ||
| bat_state.power = - raw_inv_power + inverter_state.power - home_consumption |
There was a problem hiding this comment.
Ich versuche die Rechnung noch nachzuvollziehen. Ich fasse mal zusammen, was ich verstanden habe:
- dc_in = Brutto PV Leistung
- bat_state.power = Brutto Batterieleistung
- inverter_state.power = Netto Gesamtleistung
- home_consumption = ? Vom Name her würde ich erwarten, dass wenn Ladung aus der Batterie entnommen wird, findet hoffentlich keine Einspeisung statt. Bei geringer Leistung wäre dann
home_consumption == inverter_state.power. Allerdings natürlich nicht dann, wenn der Hausverbrauch die Wechselrichterleistung, bzw die Maximalleistung der Batterie übersteigt, dann könnte diehome_consumptionauch darüber liegen. Mir ist nicht klar, was dieser Wert im Zusammenhang hier tut.
Ich nehme an im Idealfall wäre dc_in + bat_state.power == inverter_state.power, also Bruttogesamtleistung gleich Nettogesamtleistung. Es sind aber Wandlungsverluste dabei, so dass inverter_state.power etwas geringer ist, als die Summe. Das willst du so umrechnen, dass PV und Batterieleistung netto sind, also die Verluste bereits abgezogen.
Wenn ich versuche das ganze für mich nachvollziehbar in kleinen Schritten aufzuschreiben, dann käme ich auf:
power_gross = bat_state.power + dc_in
power_net = inverter_state.power
pv_fraction = dc_in / power_gross
inverter_state.power = pv_fraction * power_net
bat_state.power = (1 - pv_fraction) * power_netWenn man es kürzer machen möchte ginge auch:
bat_state.power = (bat_state.power / (bat_state.power + dc_in)) * inverter_state.power
inverter_state.power -= bat_state.powerEin paar Zwischenvariablen können in dem Fall allerdings auch helfen die Rechnung bessen nachzuvollziehen. Zwischenlösung könnte auch sein:
power_gross = bat_state.power + dc_in
bat_state.power = bat_state.power / power_gross * inverter_state.power
inverter_state.power -= bat_state.powerZumindest was die PV-Leistung angeht mache ich da das Selbe was du machst. Was das mit der home_consumption bei der Batterie auf sich hat weiß ich nur nicht...
Was mich noch etwas stört ist, dass sich die Bedeutung der Variablen bat_state und inverter_state ändert. Man muss entsprechend beim Lesen vom Quelltext den Kontext im Auge behalten, wann man auf diese Variable zugreift. Denn mal ist es die Wechselrichterleistung und mal die PV-Leistung bzw. mal die Bruttobatterieleistung und mal die Nettobatterieleistung. Da kommt wieder durch, dass ich ein großer Fan von Unveränderlichkeit (hier: Immutable objects) bin, da solche Bedeutungsänderungen damit verschwinden. Mein Vorschlag wäre bat_state umzubenennen in bat_state_net und inverter_state vom Namen her zu belassen. Dann zwei neue Variablen, was in etwa so aussehen würde:
power_gross = bat_state.power + dc_in
bat_state_gross = BatteryState(power=bat_state_net.power / power_gross * inverter_state.power)
pv_state = InverterState(power=inverter_state.power - bat_state_gross.power)Auf diese Weise ist man auch gezwungen nochmal darüber nachzudenken was eigentlich mit den Zählersummen ist. Also exported und imported. Die kommen aus KostalPlenticoreBat aus SimCount und bei KostalPlenticoreInverter aus dem Wechselrichter. Haben diese Zahlen noch eine Bedeutung, wenn du hier anders rechnen möchtest als Kostal es tut? Oder müsstest du den SimCount-Aufruf in deine set-Methoden in deinen Komponenten verschieben?
Was ist wenn es mehrere Inverter gibt? Würde das nicht dazu führen, dass von jedem WR was abgezogen wird statt nur von dem einen Hybrid?
There was a problem hiding this comment.
Über die Berechnung bin ich beim Rewriting auch gestolpert. Da ich weder den WR kenne noch das Modul geschrieben habe, habe ich mich auf das reine Rewriting beschränkt. Wenn ich es richtig verstehe, ist Deine Berechnung eine andere als die ursprüngliche oder?
Dass die Variablen einen eindeutigen Wert wiedergeben, habe ich geändert.
Bisher wurde der Zählerstand nicht angepasst, sondern ausgelesen bzw mittels Simcount ermittelt.
There was a problem hiding this comment.
Wenn ich mich vllcht. Einmischen darf, ich hatte das Modul kostal_plenticore
.py vor einiger Zeit einmal Restrukturiert und etwas modularer gestaltet. Sofern das noch die Grundlage ist, hier mein Kommentar, sonst ignorieren. Genau die gleichen Themen kamen da auch hoch und als plenticore Besitzer und vielen Testen nahe bei der Ursprungs Variante geblieben. Aus meiner Sicht, der viele Update mit dem plenticore gemacht hat und auch die release notes gelesen hat, kann ich sagen, KOSTAL scheint sich hier selber schwer zu tun den "wahren" Wert zu berechnen, da hier einige Bugfixes im laufe der Zeit ausgegeben wurden. In der aktuellen Version passen die UI Werte von Smart Meter und Wechselrichter für den Hausverbrauch nicht einmal überein. Nichts desto trotz, letztendlich bietet die Modbus Schnittstelle nicht die von KOSTAL angezeigten werden, somit muss man die Einzelwerten in den verschiedenen Modi (PV Einspeisen, PV einspeisen&Batterie laden, PV & Batterie entladen, batterie betrieb , Batterie&Grid Verbrauch, Verbrauch und Batterie laden aus Grid) des Wechselrichter berechnen, in jeder Phase berechnen sich die Verluste ggf. anders. Ohne es noch genau zu wissen, war der aktuelle Stand der repräsentativste wenn sicherlich auch hier und da nicht wahre Wert. Ich habe über Zeiten mit den Wechselrichter Ausgaben verglichen und es passte.
| def little_endian_wordorder_reader(register: int, data_type: modbus.ModbusDataType): | ||
| return tcp_client.read_holding_registers( | ||
| register, data_type, unit=71, wordorder=Endian.Little) | ||
| return little_endian_wordorder_reader |
There was a problem hiding this comment.
| def little_endian_wordorder_reader(register: int, data_type: modbus.ModbusDataType): | |
| return tcp_client.read_holding_registers( | |
| register, data_type, unit=71, wordorder=Endian.Little) | |
| return little_endian_wordorder_reader | |
| return functools.partial(tcp_client.read_holding_registers, unit=71, wordorder=Endian.Little) |
Und dann auch aus Zeile 95 heraus diese Funktion aufrufen.
|
|
||
| def read_legacy_inverter(ip1: str, ip2: str, battery: int, ip3: str, position: int) -> InverterState: | ||
| # in IP3 kann ein aufeinanderfolgende Liste enthalten sein "192.168.0.1-3" | ||
| log.debug("Kostal Plenticore: WR1: {}, WR2: {}, Battery: {}, WR3: {}".format(ip1, ip2, battery, ip3)) |
There was a problem hiding this comment.
Das Python-Logging-Framwork hat diese eingebaute Formatierungsdings... Nebenbei: Du hast die gleiche Reihenfolge wie die Parameter genommen. Deren Reihenfolge ist nehme ich an historisch gewachsen und nicht aus Sinnnhaftigkeit entstanden. Für das Logstatement mag es sinniger sein das anders zu sortieren:
| log.debug("Kostal Plenticore: WR1: {}, WR2: {}, Battery: {}, WR3: {}".format(ip1, ip2, battery, ip3)) | |
| log.debug("Kostal Plenticore: WR1: %s, WR2: %s, WR3: %s, Battery: %s", ip1, ip2, ip3, battery) |
| def update( | ||
| components: Iterable[Union[KostalPlenticoreBat, KostalPlenticoreCounter, KostalPlenticoreInverter]], | ||
| reader: Callable[[int, modbus.ModbusDataType], Any]): | ||
| request_components(components, reader) |
There was a problem hiding this comment.
Die Funktion delegiert nur an request_components. Ohne irgendetwas zu verändern. Wird das irgendwofür gebraucht? Sonst könnte diese funktion komplett inlined werden.
| with client_1: | ||
| components = [KostalPlenticoreInverter(KostalPlenticoreInverterSetup(id=1))] | ||
| if battery: | ||
| components.append(KostalPlenticoreBat(None, KostalPlenticoreBatSetup())) |
There was a problem hiding this comment.
Die Funktionsdefinition erwartet eine device_id, auch wenn sie im Legacy-Context nicht genutzt wird. Ein Type-Checker merkt das sofort ;-).
| return little_endian_wordorder_reader | ||
|
|
||
|
|
||
| def read_legacy_inverter(ip1: str, ip2: str, battery: int, ip3: str, position: int) -> InverterState: |
There was a problem hiding this comment.
Parameter position ist ungenutzt
| client_1 = modbus.ModbusTcpClient_(ip1, 1502) | ||
| reader_1 = _create_reader(client_1) |
There was a problem hiding this comment.
Die Namen client_1 und read_1 sagen mir nicht so zu. Du könntest sie client_main nennen oder so. Noch besser wäre aber das kommt in einen kleineren Scope.
Das ganze hat mich dazu gebracht generell zu überlegen ob es möglich wäre die Funktion völlig anders zu strukturieren. Hiermit bin ich rausgekommen:
def read_legacy_inverter(ip1: str, ip2: str, battery: int, ip3: str) -> InverterState:
# in IP3 kann ein aufeinanderfolgende Liste enthalten sein "192.168.0.1-3"
log.debug("Kostal Plenticore: WR1: %s, WR2: %s, WR3: %s, Battery: %s", ip1, ip2, ip3, battery)
inverter_component = KostalPlenticoreInverter(KostalPlenticoreInverterSetup(id=1))
def get_hybrid_inverter_state(ip: str) -> InverterState:
battery_component = KostalPlenticoreBat(1, KostalPlenticoreBatSetup())
with modbus.ModbusTcpClient_(ip) as client:
return request_components(
[inverter_component, battery_component], _create_reader(client), set_inverter_state=False
)
def get_standard_inverter_state(ip: str) -> InverterState:
with modbus.ModbusTcpClient_(ip) as client:
return inverter_component.update(_create_reader(client))
def inverter_state_sum(a: InverterState, b: InverterState) -> InverterState:
return InverterState(exported=a.exported + b.exported, power=a.power + b.power)
inverter_state = reduce(
inverter_state_sum,
map(get_standard_inverter_state, filter("none".__ne__, chain([ip2], ipparser(ip3)))),
get_hybrid_inverter_state(ip1) if battery else get_standard_inverter_state(ip1)
)
inverter_component.set(inverter_state)
return inverter_stateDas sind 26 Zeilen statt 20 Zeilen vorher, also leider länger. Ist es das wert? Ist es besser lesbar? Das liegt wohl im Auge des Betrachters. Du kannst ja mal mit etwas Abstand drauf schauen ob du es verständlich findest.
| KostalPlenticoreInverterSetup(id=1)).update(_create_reader(client)) | ||
| inverter_state.power += inverter_state_temp.power | ||
| inverter_state.exported += inverter_state_temp.exported | ||
| components[1].set(inverter_state) |
There was a problem hiding this comment.
components[1]? Eventuell gibt es nur eine Komponente und das hier ist nicht gesetzt. Oder es ist die Batterie. Wolltest du components[0]? Wobei es eigentlich nur eine Batterie und einen Inverter gibt. Auf die Liste könntest du auch verzichten.
| client_1 = modbus.ModbusTcpClient_(ip1, 1502) | ||
| reader_1 = _create_reader(client_1) |
There was a problem hiding this comment.
Es gibt keine konkurrierenden Namen hier. Es ginge auch ohne _1
| self.sim_counter = SimCounter(device_id, self.component_config.id, prefix="speicher") | ||
| self.component_info = ComponentInfo.from_component_config(self.component_config) | ||
|
|
||
| def update(self, reader: Callable[[int, ModbusDataType], Any]) -> BatState: |
There was a problem hiding this comment.
Die Methode updated nichts. Sie liest nur den Status. Genau das gleiche in KostalPlenticoreInverter. Vielleicht also besser read oder readState oder getState oder so?
| openwbDebugLog ${DMOD} 2 "RET: ${ret}" | ||
| speicherleistung=$(<"${RAMDISKDIR}/speicherleistung") | ||
| openwbDebugLog ${DMOD} 1 "BattLeistung: ${speicherleistung}" | ||
| # wr_plenticore will fetch data for both inverter and counter and there is nothing left for us to do. |
There was a problem hiding this comment.
Sollte wahrscheinlich "both inverter and battery" oder velleicht noch besser "both PV and battery" heißen?
| frequency=frequency | ||
| ) | ||
|
|
||
| def get_imported_exported(self, state): |
There was a problem hiding this comment.
Vom Namen her hätte ich erwartet, dass die Methode einfach imported und exported zurückgibt. Das ist nur indirekt der Fall. Wie wäre update_imported_exported?
|
|
||
| def get_hybrid_inverter_state(ip: str) -> InverterState: | ||
| battery_component = KostalPlenticoreBat(1, KostalPlenticoreBatSetup()) | ||
| with modbus.ModbusTcpClient_(ip) as client: |
There was a problem hiding this comment.
Ich war in meiner letzten Runde da etwas schnell. Mir ist noch aufgefallen, dass du vorher port 1502 mitgegeben hast. Das sollte vermutlich erhalten bleiben.
| return inverter_state | ||
|
|
||
|
|
||
| def read_legacy_counter(ip1: str, ip2: str, battery: int, ip3: str, position: int) -> None: |
There was a problem hiding this comment.
Der Aufruf kommt von hier?:
Dann fehlen da aber Parameter.
| with client: | ||
| counter_state = counter_component.get_values(reader) | ||
| bat_power = KostalPlenticoreBat(None, KostalPlenticoreBatSetup()).update(reader).power | ||
| inverter_power = read_legacy_inverter(ip1, ip2, battery, ip3).power |
There was a problem hiding this comment.
In dem Fall würde das Update zwei mal durchgeführt werden. Schlimm ist das vermutlich nicht. Wahrscheinlich haben das Setup so sowieso nur 3 Leute ;-). Es könnte aber ein Gedanke wert sein, den Teil auch an die "große" Abfrage anzuhängen.
|
@skl77 Hast Du die Möglichkeit, den PR vor dem Mergen zu testen? Die Änderungen zur Umstellung von der bisherigen Struktur auf die neue waren doch sehr grundlegend. |
|
Ja, das kann ich machen. Werde Sonntag mal meine entwicklung Maschine wieder in Betrieb nehmen |
| ret=$? | ||
|
|
||
| openwbDebugLog ${DMOD} 2 "RET: ${ret}" | ||
| bash "$OPENWBBASEDIR/packages/legacy_run.sh" "modules.kostal_plenticore.device" "counter" "${kostalplenticoreip}" "" "0" "" "${kostalplenticorehaus}" >> "$MYLOGFILE" 2>&1 |
There was a problem hiding this comment.
Du hast die fehlenden Parameter jetzt hinzugefügt. Ist natrürlich auch eine Möglichkeit. Wobei du dann meines Erachten "none" statt "" verwenden. Einfacher fände ich es allerdings, wenn du die ungenutzten Parameter in Python entfernst.
|
Habe mal auf die Schnelle das Modul angetastet, ich kann schon mal soviel sagen, da passt einiges funktional noch nicht. Update kommt .. |
|
@skl77 Hast Du herausgefunden, wo der Fehler liegt? Oder hast Du einen Logauszug und die Werte dazu, die Du erwartet hast? |
| def create_bat_component(component_config): | ||
| return KostalPlenticoreBat(device_config.id, component_config) | ||
|
|
||
| def create_counter_component(component_config): | ||
| return KostalPlenticoreCounter(device_config.id, component_config) | ||
|
|
||
| def create_inverter_component(component_config): | ||
| return KostalPlenticoreInverter(component_config) |
There was a problem hiding this comment.
Der Parameter component_config in diesen drei Funktionen braucht noch eine Type-annotation. Das ist der Grund für den Fehler hier im Forum. (So wie in meinem ursprünglichen Beispiel dazu)
LKuemmel
force-pushed
the
kostal_plenticore
branch
from
af07d51 to
2d4dee3
Compare
@yankee42 Einige Punkte sind durch die Umstellung auf ConfigurableDevice noch offen:
Wie wird die ID für simcount übergeben? Diese müsste in die init-Methode der Komponenten.
Wie wird der tcp_client für die update_components-Funktion übergeben?