Solaredge: consider synergy units

[LKuemmel]

No description provided.

[yankee42]

Die logging-Funktionen unterstützen format-String direkt per varargs:

Suggested change

	log.debug("Synergy Units: "+str(self.synergy_units))
	log.debug("Synergy Units: %s", self.synergy_units)

Das vermeidet den cast und sieht finde ich etwas schöner aus. Es hat sogar einen kleinen Performance-Vorteile: Wenn das Loglevel auf einem kleineren Level als Debug steht, dann wird der String garnicht erst gebaut.

[yankee42]

Ich habe eine ganze Weile gebraucht um zu verstehen was hier passiert. Das geht auch einfacher. Hier mein Gegenvorschlag: Ignorieren. Siehe mein eigener Antwortkommentar.
~~

meters = [None]*3  # type: List[Union[SolaredgeExternalInverter, SolaredgeCounter, None]]
for component in self.components.values():
    if isinstance(component, (SolaredgeExternalInverter, SolaredgeCounter)):
        meters[component.component_config.configuration.meter_id-1] = component

for meter_id, meter in enumerate(meters, start=1):
    if meter is not None:
        log.debug(
            "%s: internal meter id: %d, synergy units: %s", meter.component_config.name, meter_id, synergy_units
        )
        meter.regs = MeterRegisters(meter_id, synergy_units)

Sind dann nurnoch 11 Zeilen statt 27 ;-). Wenn ich nichts übersehen habe tut es das selbe...

[yankee42]

Ich habe gerade nochmal drüber geschaut und doch, ich glaube ich habe was übersehen. Die meter_ids sollten gerade lückenlos sein. Das macht es sogar noch einfacher:

for meter_id, meter in enumerate(sorted(
    (
        component for component in self.components.values()
        if isinstance(component, (SolaredgeExternalInverter, SolaredgeCounter))
    ),
    key=lambda component: component.component_config.configuration.meter_id
), start=1):
    log.debug(
        "%s: internal meter id: %d, synergy units: %s", meter.component_config.name, meter_id, synergy_units
    )
    meter.regs = MeterRegisters(meter_id, synergy_units)

Eventuell ist das Konstrukt aber doch schöner zu lesen, wenn man es wieder etwas zerteilt:

sorted_metered_components = sorted(
    (c for c in self.components.values() if isinstance(c, (SolaredgeExternalInverter, SolaredgeCounter))),
    key=lambda c: c.component_config.configuration.meter_id
)
for meter_id, meter in enumerate(sorted_metered_components, start=1):
    log.debug(
        "%s: internal meter id: %d, synergy units: %s", meter.component_config.name, meter_id, synergy_units
    )
    meter.regs = MeterRegisters(meter_id, synergy_units)

Zur Erklärung: Ich habe das ganze so verstanden, dass es darum geht nicht konfigurierte Zähler raus zu nehmen. Also wenn Zähler 1 und 3 konfiguriert sind, wird aus der 3 eine 1. Wenn 2 und 3 konfiguriert sind, wird aus der 2 eine 1 und aus der 3 eine 2. Um das zu erreichen müssen wir nur die Componenten nach der meter_id sortieren und danach neu durch nummerieren.

[LKuemmel]

Das Prinzip hast Du richtig verstanden. Aber reicht es nicht einfach mit meters = list(filter(None, meters)) die Lücken zu entfernen? Das Sortieren passiert doch schon durch das Eintragen an der richtigen Position in der Liste meters[component.component_config.configuration.meter_id-1] = component

Ich habe einen Test dafür geschrieben, Commit kommt gleich.

[yankee42]

Ja klar, das kannst du auch machen. Ist letztlich geschmackssache. Der Effekt ist der selbe.

Eventuell könnte man die Zeile

meters[component.component_config.configuration.meter_id-1] = component

noch ändern zu:

index = component.component_config.configuration.meter_id-1
if meters[index]:
    raise FaultState.error("Würdest du bitte damit aufhören zwei Komponenten mit gleicher meter-id zu konfigurieren? Wie stellst du dir das eigentlich vor?");
meters[index] = component

[yankee42]

Suggested change

	if (isinstance(component, SolaredgeInverter) or
	isinstance(component, SolaredgeExternalInverter)):
	if (isinstance(component, (SolaredgeInverter, SolaredgeExternalInverter)):

[yankee42]

Hier könntest du in einem Kommentar die Spec zitieren:

For 2-unit three phase inverters with Synergy technology, add 50 to the default addresses.

For 3-unit three phase inverters with Synergy technology, add 70 to the default addresses.

[yankee42]

Wo kommen die Zahlen her? Von der Spec her hätte ich 122/296/470 erwartet (oder eventuell 121/295/469).

[LKuemmel]

Der Default-Offset (122) ist in den Registern in der Tabelle schon miteinberechnet. Ich habe in der MeterRegisters-Klasse, die Register genommen, die auch in der Tabelle stehen und deshalb den Default-Offset von den angegebenen Offsets subtrahiert.

[yankee42]

Suggested change

	log.debug("Option "+str(option))
	log.debug("Option %s", option)

[yankee42]

Suggested change

	for (name, value) in self.__dict__.items():
	for name, value in self.__dict__.items():

[yankee42]

Persönlich würde ich emphelen solche Abkürzungen zu vermeiden. registers erscheint mir klarer als regs und man kommt sich beim Vorlesen des Quelltexts nicht so komisch vor. Dank Code-Completion der IDE ist es auch nicht wirklich mehr Tippaufwand.

[yankee42]

Vielleicht besser SolaredgeMeterRegisters? Gut, kann man wieder drüber streiten, denn die Klasse ist bereits im Modul solaredge. Nur sieht man das Modul häufig nicht und mit dem expliziten Namen wird sofort klar, dass das nichts allgemeines ist. Die anderen Klassen heißen auch SolaredgeCounter etc.

[yankee42]

Option ist von zwei Zeilen darüber ein Array mit 8 int? Das kann also nicht matchen?

[yankee42]

filter(None, meters) erzeugt einen Generator, der "on demand" die Liste meters durchgeht und dabei alle Einträge die nicht "truthy" sind überspringt. Das list zwingt Python dann dazu alle Elemente abzurufen und einzeln im Speicher in einer Liste abzulegen. Nicht nötig. meters = filter(None, meters) tut alles was benötigt ist.

Ich würde dem ganzen noch entweder einen anderen Namen geben oder inlinen. Also so:

used_meters = filter(None, meters)

Oder so:

for meter_id, meter in enumerate(filter(None, meters), start=1):

Das reduziert das Risiko, dass man übersieht, dass die Variable nochmal überschrieben wird.

[yankee42]

None wurde bereits in Zeile 96 rausgefiltert. Diese Bedingung kann niemals false sein und kann daher ersatzlos gestrichen.

[yankee42]

Du bist mit dem test auf unterschiedlichen Ebenen unterwegs. Das zeigt sich schon im Namen. Normalerweise haben wir eine Datei something.py und dazu eine Datei something_test.py die das testet, was in erster Datei steht. Selbst wenn ich das test in test_meter_registers.py nach hinten stelle gibt es keine meter_registers.py. Du könntest die Datei meter_test.py nennen, aber es würde nicht ganz stimmen, denn du testest einiges mit, was in Device liegt. device_test.py wäre besser, weil das die Ebene ist auf der du ansetzt. Aber dafür geht der Test ganz schön ins Detail was die Register betrifft.

Um das ganze besser testbar zu machen würde ich den Test splitten in die Aspekte die du testen willst. Ich sehe da zwei Aspekte:

1. Das was die Funktion _update_regs tut: Komponenten auf Basis der meter_id ein Register-Objekt zuweisen
2. Das was die Klasse SolaredgeMeterRegisters tut: Die richtigen Register berechnen.

Das ganze dann auch in zwei Dateien.

Für die Funktion _update_regs machst du dir das Leben leichter, wenn du die Abhängigkeit von self entfernst. Ggf. indem du eine Funktion außerhalb der Klasse daraus machst oder einen @staticmethod. Kann dann zum Beispiel so aussehen:

def set_component_registers(components: Iterable[Any], synergy_units: int) -> None:
    # ...

Wenn du das so testest, brauchst du im Test die Klasse Device nicht instantieren, musst entsprechend auch den Modbus-Client nicht monkey-patchen und kannst die Funktion trotzdem richtig gut auf Herz und Nieren prüfen. Es reicht dann auch, wenn du prüfst, dass meter_id und synergy_units and SolaredgeMeterRegisters "durchgeschliffen" wird. Zum Beispiel indem du SolaredgeMeterRegisters monkeypatchst - in etwa so (pseudocode):

def test_meter_registers_calculation(monkeypatch):
    # setup
    monkeypatch.setattr(modules.solaredge.device, "SolaredgeMeterRegisters", Mock(side_effect=lambda *args: args))
    counter = Mock(spec=SolaredgeCounter)
    external_inverter = Mock(spec=SolaredgeExternalInverter)
    components = ["ignore me", counter, external_inverter]

    # execution
    set_component_registers(components, synergy_units=1)

    # evaluation
    assert external_inverter.registers == (2, 1)

Das Mock von SolaredgeMeterRegisters gibt anstelle des instantierten Objekts die Argumentliste als tuple zurück. Da kannst du im Nachhinein prüfen ob ein Objekt in registers gespeichert wurde, welches aus einem Aufruf von SolaredgeMeterRegisters mit den richtigen Argumenten kam. Ohne dass du die ganze Logik, die sich in SolaredgeMeterRegisters befindet mittesten musst. Das kann dann in einen seperaten Test.

[yankee42]

Hintergrund

Die Idee mit einer Params Klasse und dem name-Parameter habe ich angefangen um klar zu kommunizieren, was der Test testet. Der Name hat dabei einen doppelten Zweck:

1. Er ist ein Kommentar für Leser, der die Intention darlegt
2. Wenn der Test fehlschlägt findet man den Test schneller.

Erstmals verwendet habe ich das im simcount-test. Sieht da so aus:

Params(name="Ascending from zero:",
       seconds=1, power_previous=0, power_present=10, expected_energy=(5, 0)),
Params(name="Ascending from more than zero:",
       seconds=1, power_previous=10, power_present=11, expected_energy=(10.5, 0)),
# ...

In dem Fall beschreibt der name was ich testen will. Im ersten Beispiel eine Situation in der die Leistung von null beginnt und ansteigt. Die genauen Zahlen sind dabei nicht so wichtig. Wenn ich absichtlich einen Test ändere, so dass er kaputt ist sieht das in IntelliJ zum Beispiel so aus:

Eine ähnliche Ausgabe kommt sicherlich auch in VisualStudio oder hier in den Github-Actions. So findet man schnell welcher Test schief gelaufen ist.

Hier:

In deinem Fall hier wird der Text auch helfen den Fall zu finden, aber er fügt keine Informationen hinzu was du dir bei dem Test gedacht hast. Das muss auch nicht zwangsläufig immer so sein, manchmal gibt es eben nicht viel mehr zu sagen, als die reinen Parameter. Aber dann ist doof, dass du mit "meter id 1, synergy units 1", meter_id=1, synergy_units=1 die exakt gleichen Infos zweimal gibst: Einmal mit den Parametern und einem im Text.

Variante @auto_str

Eine recht einfache Möglichkeit das zu verschönern ist an die Klasse Params mit @auto_str zu arbeiten:

@auto_str
class Params:
    def __init__(self,
                 meter_id: int,
                 synergy_units: int,
                 expected_power_register: int):
        self.meter_id = meter_id
        self.synergy_units = synergy_units
        self.expected_power_register = expected_power_register


cases = [Params(meter_id=1, synergy_units=1, expected_power_register=40206),
         # ...
         ]


@pytest.mark.parametrize("params", cases, ids=str)
def test_meter(params: Params):
    # ...

Sieht dann im Ergebnis so aus:

Vielleicht nicht perfekt, weil unnötigerweise Params noch dran steht und expected_power_register, aber dafür ist es einfach sehr einfach.

Variante NamedTuple

Eine weitere Variante wäre ein NamedTuple zu verwenden:

Params = NamedTuple("Params", [("meter_id", int), ("synergy_units", int), ("expected_power_register", int)])

cases = [Params(meter_id=1, synergy_units=1, expected_power_register=40206),
         # ...
         ]

Der Effekt ist gleich wie die Variante oben mit @auto_str. Nur etwas kompakter. Alles nur geschmackssache ;-).

Variante pytest-"Standard"

Natürlich kann man auch wie in pytest nicht unüblich direkt tuples verwenden. Man muss nur aufpassen, dass man den Überblick nicht verliert welcher Parameter wo landet:

@pytest.mark.parametrize(
    "meter_id,synergy_units,expected_power_register",
    [
        (1, 1, 40206),
        # ...
    ]
)
def test_meter(meter_id: int, synergy_units: int, expected_power_register: int):
    # ...

Fazit

Es gibt viele Möglichkeiten (noch viel mehr ist denkbar), ich würde aber emphelen eine Variante zu nehmen, wo die "Test-ID" (hier: der "name") entweder eine Beschreibung der Intention ist, oder die Test-ID automatisch bestimmen lassen.

[yankee42]

Der Name der Datei devcie_test.py enthält einen Tippfehler.

[yankee42]

Der Test ist so schon garnicht so schlecht. Er ist kurz, strukturiert und einen recht überschaubaren Bereich den er testet. Es aber auch noch ein paar Kleinigkeiten, über die man nachdenken könnte:

1. Die Beschreibung der cases geben nicht her, was die Intention des Tests ist, sondern wiederholen nur die Parameter. Sinniger wäre es dann nur die Parameter anzugeben (siehe auch Kommentar zur meter_test.py). Noch besser wäre eine klare Beschreibung
2. Es ließe sich noch klarer eingrenzen, was der Test testet.

Punkt 1 ist relativ einfach. Punkt 2... Ist etwas schwieriger. Ich habe es trotzdem mal versucht. Mein Gedanke ist, dass der Paramter synergy_units nur durchgeschliffen wird. Interessant ist eigentlich eher was mit der meter_id passiert. Die wird neu zugewiesen. Daraus könnte man zum Beispiel folgenden Test ableiten:

Params = NamedTuple("Params", [("configured_meter_ids", List[int]), ("effective_meter_ids", List[int])])


@pytest.mark.parametrize(["params"], [
    pytest.param(
        Params(configured_meter_ids=[1, 2], effective_meter_ids=[1, 2]),
        id="ids unchanged if meter_ids are continuous starting from 1"
    ),
    pytest.param(
        Params(configured_meter_ids=[2, 3], effective_meter_ids=[1, 2]),
        id="ids move forward if not starting at 1"
    ),
    pytest.param(
        Params(configured_meter_ids=[1, 3], effective_meter_ids=[1, 2]),
        id="gaps in ids are closed"
    )
])
def test_set_component_registers_assigns_effective_meter_ids(monkeypatch, params: Params):
    # setup
    monkeypatch.setattr(
        device, "SolaredgeMeterRegisters", Mock(side_effect=lambda internal_meter_id, _: internal_meter_id)
    )
    components_list = [
        Mock(spec=SolaredgeCounter, component_config=Mock(configuration=Mock(meter_id=meter_id)))
        for meter_id in params.configured_meter_ids
    ]
    components_map = {"component" + str(index): component for index, component in enumerate(components_list)}

    # execution
    device.Device.set_component_registers(components_map, synergy_units=1)

    # evaluation
    assert [component.registers for component in components_list] == params.effective_meter_ids

Das gute daran: Die Testbeschreibung ist sehr konkret. Einmal im Namen der Testmethode: test_set_component_registers_assigns_effective_meter_ids. Das sagt sehr spezifisch aus worum es geht: Die Neuzuweisung der meter_ids. Und auch die Beschreibungen habe ich angepasst.

Es zeigt sich bei dem Test, dass es noch unschön ist, dass die Funktion set_component_registers gerne ein Dict hätte. Ich muss im Test mit einer Liste arbeiten, weil ich eine definierte Sortierung brauche. Dann muss ich mit components_map = ... erst ein Dict daraus machen, nur damit die Funktion set_component_registers sofort .values() darauf aufruft und die Keys ignoriert. Das ist auch in deinem Test schon unschön, aber so wie ich es hier aufgezogen habe fällt das noch mehr auf. Mein Rat wäre die Signatur zu ändern auf def set_component_registers(components: Iterable[solaredge_component_classes], synergy_units: int) -> None:. Der Aufruf in der device.py:80 wird dadurch unwesentlich länger: self.set_component_registers(self.components.values(), self.synergy_units), der Test kann dadurch aber nochmal vereinfacht werden. Außerdem wird dann die Reihenfolge definiert beibehalten und man könnte auch einen Testfall:

pytest.param(
    Params(configured_meter_ids=[2, 1], effective_meter_ids=[2, 1]),
    id="ids unchanged if meter_ids are continuous starting from 1, ignoring order"
),

hinzufügen.

Dann könnte man noch überlegen ob ein Test sinn ergibt, der prüft, welche Arten von Komponenten geprüft werden:

@pytest.mark.parametrize("type,should_use", [
    (SolaredgeCounter, True),
    (SolaredgeExternalInverter, True),
    (SolaredgeBat, False),
    (SolaredgeInverter, False),
])
def test_set_component_registers_ignores_wrong_types(monkeypatch, type: Type, should_use: bool):
    # setup
    monkeypatch.setattr(
        device, "SolaredgeMeterRegisters", Mock(side_effect=lambda *args: True)
    )
    component = Mock(spec=type, component_config=Mock(configuration=Mock(meter_id=1)))
    components = {"component1": component}

    # execution
    device.Device.set_component_registers(components, synergy_units=1)

    # evaluation
    assert hasattr(component, "registers") == should_use

Das ganze ist allerdings schon optimieren auf einem sehr hohen Niveau. Ich bin das ganze als "Denksport" so durchgegangen und bin jetzt in meiner Implementierung voll drin. Ob es besser verständlich und nachvollziehbar für künftige Entwickler (bzw. das zukünftige selbst) ist, welche den Code warten müssen ist daher etwas, was du entscheiden müsstest auf Basis davon wie nachvollziehbar das für dich ist.

[yankee42]

Die beiden Funktionen sind sehr ähnlich. Man könnte diese so zusammenfassen:

def read_scaled(type: ModbusDataType, address: int, count: int):
    return scale_registers(
        self.__tcp_client.read_holding_registers(
            address,
            [type] * count + [ModbusDataType.INT_16],
            unit=self.component_config.configuration.modbus_id)
    )

read_scaled_int16 = functools.partial(read_scaled, ModbusDataType.INT_16)
read_scaled_uint32 = functools.partial(read_scaled, ModbusDataType.UINT_32)

Die Funktion ist allerdings auch noch doppelt in der counter.py. Ggf. könnte man zum Beispiel folgende Funktion in der scale.py ergänzen:

def create_scaled_reader(client: modbus.ModbusTcpClient_, modbus_id: int, type: ModbusDataType):
    def scaled_reader(address: int, count: int):
        return scale_registers(
            client.read_holding_registers(address, [type] * count + [ModbusDataType.INT_16], unit=modbus_id)
        )
    
    return scaled_reader

Und dann geht in den beiden Dateien:

read_scaled_int16 = create_scaled_reader(
    self.__tcp_client, self.component_config.configuration.modbus_id, ModbusDataType.INT_16
)
read_scaled_uint32 = create_scaled_reader(
    self.__tcp_client, self.component_config.configuration.modbus_id, ModbusDataType.UINT_32
)