rewrite isss daemon

[LKuemmel]

Komplette Überarbeitung des ISSS-Daemons:

• Lesen und Schreiben der Ladepunkt-Daten in eigenes Modul ausgelagert. Dies kann bei der Duo für LP 1 und LP 2 initalisiert werden.
• Auslesen des SDM-Zählers erfolgt mit den Methoden aus dem Package-Modul
• neues Modul für den B32 Zähler hinzugefügt
• Lesen und Schreiben der EVSE-Register in eigenem Modul
• Phasenumschaltung und CP-Unterbrechung erfolgt in Threads und blockieren nicht mehr den Daemon.
• Die Buchse erbt die Klasse des internen Ladepunkts und fügt Methoden zum Öffnen und Schließen des Aktors hinzu.

Wenn der ISSS-Daemon stabil läuft, kann auch der redundante Code in buchse.py und autoevse.py damit ersetzt werden.

[yankee42]

Habe nur kurz überflogen was es neues gibt... Hier ein paar Kleinigkeiten die mir ins Auge gesprungen sind.

[yankee42]

Stattdessen from modules.common.store import ramdisk_read, ramdisk_write?

[yankee42]

Der try/except-Code wiederholt sich in der Klasse viel. Ein decorator könnte helfen das ganze zu kürzen (ungetestet):

def exceptions_to_fault_state(delegate: Callable):
    def wrapper(*args, **kwargs):
        try:
            delegate(args, kwargs)
        except Exception as e:
            if isinstance(e, FaultState):
                raise
            else:
                raise FaultState.error(__name__ + " " + str(type(e)) + " " + str(e)) from e

    return wrapper


class B32:
    def __init__(self, modbus_id: int, client: modbus.ModbusTcpClient_) -> None:
        self.client = client
        self.id = modbus_id

    @exceptions_to_fault_state
    def get_imported(self) -> float:
        return self.client.read_holding_registers(0x5000, ModbusDataType.UINT_64, unit=self.id) / 1000

    @exceptions_to_fault_state
    def get_frequency(self) -> float:
        return self.client.read_holding_registers(0x5B2C, ModbusDataType.INT_16, unit=self.id) / 100

    # ...

[LKuemmel]

Ich habe die Fehlermeldung in den Kontextmanager des ModbusClients gepackt, der wird ja ohnehin aufgerufen.

[yankee42]

Dadurch ändert sich aber das was __name__ produziert.

[yankee42]

Zumindest das

if currents[0] > 3:
    phases_in_use = 1

Kann wohl weg, denn phases_in_use ist eh schon 1 ;-).

Ist es denkbar, dass nur phase 0&2 "in use" sind? Dann wären es zwei Phasen, würde bei der Logik hier aber als drei Phasen gewertet werden. Ich vermute dass irgend ein Standard das verbietet und das daher kein Thema ist, aber es ist mir beim drüberlesen ins Auge gefallen und ich wollte es zumindest mal anmerken.

Alternativen könnten sein:

phases_in_use = 0
for current in currents:
    if current > 3:
        phases_in_use += 1

Zu lang? Nun ja, es ist python, das geht auch kürzer ;-):

phases_in_use = sum(1 for current in currents if current > 3)

[yankee42]

Musste ich jetzt etwas drüber nachdenken mit der doppelten Verneinung. Einfacher wäre:

Suggested change

	def cooldown_neccessary(self) -> bool:
	return False if self.actor_moves < 10 else True
	def cooldown_neccessary(self) -> bool:
	return self.actor_moves >= 10

[yankee42]

Im Falle von new_phases != 1 stimmt der return Type nicht. Es kommt dann kein Tuple, sondern ein int.

[LKuemmel]

Ich teste das nochmal...

[yankee42]

Der Nerd in mir will folgendes vorschlagen:

Suggested change

	files.charge_points[self.num].is_charging.write(cp_state.charge_state)
	files.charge_points[self.num].voltages.write(cp_state.voltages)
	files.charge_points[self.num].currents.write(cp_state.currents)
	files.charge_points[self.num].energy.write(cp_state.imported)
	files.charge_points[self.num].is_plugged.write(cp_state.plug_state)
	files.charge_points[self.num].power.write(cp_state.power)
	charge_point = files.charge_points[self.num]
	charge_point.is_charging.write(cp_state.charge_state)
	charge_point.voltages.write(cp_state.voltages)
	charge_point.currents.write(cp_state.currents)
	charge_point.energy.write(cp_state.imported)
	charge_point.is_plugged.write(cp_state.plug_state)
	charge_point.power.write(cp_state.power)

Ist dann nurnoch 1x statt 6x ein Zugriff auf files.charge_points[self.num] und spart entsprechend Objekterzeugungen ein. Der Performancevorteil dürfte allerdings kaum messbar ausfallen...

[benderl]

Ich glaube nicht, dass die Methode in jedem einzelnen Modul Sinn macht. Das kann man doch auslagern und dann nur importieren? Würde rein inhaltlich z. B. gut in modules.common.fault_state passen.

[LKuemmel]

Dann hat man auch wieder das Problem, dass __name__='fault_state' ist.

Anderer Ansatz: Muss an dieser Stelle die Exception gefangen werden?
Es gibt nicht so viele Fehlermöglichkeiten. Die Modbus-Exceptions werden im Modbus-Modul gefangen und alle anderen Module rufen die Zähler mit Single- bzw MultiComponentUpdateContext auf, dort werden die Fehler auch entsprechend behandelt.
Nur get_values im chargepoint_module ruft die Zählermethoden ohne SingleComponentUpdateContext auf, da der Fehler nach oben weitergereicht werden muss.

Suggested change

	raise e
	raise e if isinstance (e, FaultState) else FaultState.from_exception(e)

[yankee42]

Dann hat man auch wieder das Problem, dass __name__='fault_state' ist.

Da kann man einen Parameter draus machen:

def exceptions_to_fault_state(module_name: str):
    def decorate(delegate: Callable):
        def wrapper(*args, **kwargs):
            try:
                return delegate(*args, **kwargs)
            except Exception as e:
                if isinstance(e, FaultState):
                    raise
                else:
                    raise FaultState.error(module_name + " " + str(type(e)) + " " + str(e)) from e
        return wrapper
    return decorate


@exceptions_to_fault_state(__name__)
def function_to_decorate():
    print("Here some stuff happens")

Anderer Ansatz: Muss an dieser Stelle die Exception gefangen werden? Es gibt nicht so viele Fehlermöglichkeiten. Die Modbus-Exceptions werden im Modbus-Modul gefangen und alle anderen Module rufen die Zähler mit Single- bzw MultiComponentUpdateContext auf, dort werden die Fehler auch entsprechend behandelt. Nur get_values im chargepoint_module ruft die Zählermethoden ohne SingleComponentUpdateContext auf, da der Fehler nach oben weitergereicht werden muss.

Jo stimmt. Kann vielleicht auch ersatzlos gestrichen. Das ist natürlich noch einfacher.

[hhoefling]

Was wird der neue isss.py besser können als der alte (den sogar ich als Python-Dummy verstehen kann) ?
Bringt die gesteigerte Komplezität (und Zeilenzahl) Vorteile?

[benderl]

War die Frage ernst gemeint?
Entfernen von doppeltem Code und Modularisierung bringt eine wesentlich bessere Struktur und leichtere Pflege.

[yankee42]

Habe mir nochmal etwas mehr angesehen...

[yankee42]

Die Verwendungen dieser Funktion sind meine ich jetzt weg... Dann könnte auch die Funktion selbst weg.

[yankee42]

Ich finde das if hier recht sperrig und nicht ganz so leicht nachzuvollziehen. Ich habe etwas darüber nachgedacht wie es vielleicht übersichtlicher sein könnte. Eine Idee wäre den EvseState mit den nötigen Metainformationen aufzumotzen:

class EvseState(IntEnum):
    READY = (1, False, False)
    EV_PRESENT = (2, True, True)
    CHARGING = (3, True, True)
    CHARGING_WITH_VENTILATION = (4, True, True)
    FAILURE = (5, None, None)

    def __new__(cls, num: int, plugged: Optional[bool], charge_enabled: Optional[bool]):
        member = int.__new__(cls, num)
        member._value_ = num
        member.plugged = plugged
        member.charge_enabled = charge_enabled
        return member

Das würde natürlich bedingen, dass ich richtig verstanden habe wofür die Stati stehen. Meine Interpretation ist, dass bei READY nicht los ist, bei EV_PRESENT ein Fahrzeug angesteckt ist aber Laden deaktiviert ist. Bei CHARGING und CHARGING_WITH_VENTILATION (was immer da der Unterschied ist?) wäre Laden aktiviert, aber es könnte sein, dass der Ladestrom auf Null steht und dann lädt es trotzdem nicht. Daher habe ich dem Status dem Name charge_enabled gegeben, womit kommuniziert wird, dass Laden prinzipiell aktiv ist, aber die Ladestärke 0 sein könnte. FAILURE kann nehme ich an alles sein.

Wenn diese Annahmen korrekt sein sollten, würde die Klasse EvseState so wie oben besser beschreiben. Die Bedingung hier würde auch einfacher:

set_current, _, state_number = self.client.read_holding_registers(...)
state = EvseState(state_number)
if state == EvseState.FAILURE:
    raise ...
plugged = state.plugged
charging = set_current > 0 if state.charge_enabled else False
return plugged, charging, set_current

[yankee42]

Warum soll man das zum debuggen brauchen?

[LKuemmel]

VS Code findet sonst die Module aus dem packages-Ordner nicht... kann dann noch weg, wenn alles funktioniert.

[yankee42]

Ich denke das Problem wird dich noch häufiger verfolgen. Daher wäre es sinnvoll das richtig zu lösen.

Hier (und in diversen anderen Referenzen finde ich es genau so) schreibt jemand man könnte zur settingss.json sowas hinzufügen:

"terminal.integrated.env.osx": {
    "PYTHONPATH": "${workspaceFolder}/src",
},
"terminal.integrated.env.linux": {
    "PYTHONPATH": "${workspaceFolder}/src",
},
"terminal.integrated.env.windows": {
    "PYTHONPATH": "${workspaceFolder}/src",
},

Bei uns ist es natürlich "packages" statt "src".

[benderl]

Ich habe gerade in meiner Entwicklungsumgebung auch damit gekämpft. Die Lösung war letztendlich relativ einfach:
Direkt im Projektordner eine Datei .env mit folgendem Inhalt anlegen: PYTHONPATH=packages
Sofort läuft pytest im VS Code Terminal durch und auch der Test-Explorer zeigt keine Fehler mehr.
Wenn das bei euch auch so läuft, kommt die Datei ins Repo.

[LKuemmel]

Ich habe in launch.json "env": {"PYTHONPATH": "/var/www/html/openWB/packages"} ergänzt.

[benderl]

Aber nicht wirklich den absoluten Pfad?

[yankee42]

Hier wird eine Liste erzeugt, mit der dann nichts passiert. Das ist unintuitiv. Besser wäre:

for i in range(0, 3):
    self.pub_values_to_1_9(topic[i], value[i])

Oder, falls man sich darauf verlassen kann, dass topic und value immer 3 Elemente haben:

for i in zip(topic, value):
    self.pub_values_to_1_9(*i)

[yankee42]

In Zeile 84 steht das Gleiche nochmal

[yankee42]

Sieht suspekt aus. Sollte das nicht vielleicht ["VPhase1", "VPhase2", "VPhase3"] heißen?

Wenn nein würde ["VPhase1"]*3 deutlich zeigen, dass es Absicht ist, dass der Wert sich exakt wiederholt.

[yankee42]

Aufgrund der ganzen indices ist die Schleife recht kryptisch. Wenn du eine Klasse oder "named tuple" verwendest, kannst du über Namen gehen, dann wird es verständlicher. Eine andere Alternative wäre meter[0] und meter[1] einen Namen zu geben. Das erlaubt Python ganz elegant mit einem destructuring-Statement:

for meter_type, modbus_id in meter_list:
    meter_client = meter_type(modbus_id, serial_client)

Etwas flüssiger fände ich btw als Variablenname zu lesen: meters statt meter_list.

[yankee42]

Eigentlich geschickt das Meter anhand eines Registers zu bestimmen. So sparst du dir eine Konfiguration dafür. Gleichzeitig aber auch mutig. Wenn es nicht der erwartete Zähler ist, dann wird eben irgendetwas von einem anderen Zähler ausgelesen. Für den B32 habe ich leider nicht auf die Schnelle eine Dokumentation der Register gefunden, um mal zu schauen ob das jetzt schon das Potential einer Fehlerkennung vorliegt. Spätestens jedoch wenn irgendjemand die Liste der Zähler erweitert kommt das Potential auf, dass hiermit irgendetwas ganz anderes erkannt wird, weil der Wert zufällig auch > 20 ist.

Solange es keinen Register gibt in dem eine Modellbezeichnung steht wird es schwierig sein Code zu schreiben der sicher ist. Aber ein Kommentar an der Stelle der erklärt welcher Register hier bei dem SDM630 ausgelesen und warum das so hinkommt wäre gut um zukünftige Entwickler die den Code erweitern (oder nur auf Fehlersuche debuggen) zu helfen.

(Ist es nicht eigentlich so, dass über die modbus_id das ganze eindeutig ist und der Test eher sein müsste ob eine Exception fliegt oder nicht? Also ob auf der id irgendwas antwortet?)

[yankee42]

Das soll wahrscheinlich dafür da sein, den Standby-Stromverbrauch nicht als Ladestrom zu erkennen. Kann man natürlich so machen. Deutlicher wäre es wenn es dafür eine Konstante gäbe:

if power < PLUG_STANDBY_POWER_THRESHOLD:
    power = 0

Damit wäre dokumentiert warum die Abfrage gemacht wird: Um festzustellen ob es sich um einen reinen Standby-Verbrauch handelt...

[yankee42]

Den Unterschied zwischen den beiden Methoden muss man mit der Lupe suchen. Dass die sich die Länge vom sleep unterscheidet ist mir nicht klar ob Absicht ist...

Alternative könnte sein:

def __open_actor(self):
    self.__set_actor(open=True)

def __close_actor(self):
    self.__set_actor(open=False)

def __set_actor(self, open: bool):
    GPIO.output(23, GPIO.LOW if open else GPIO.HIGH)
    GPIO.output(26, GPIO.HIGH)
    time.sleep(3)
    GPIO.output(26, GPIO.LOW)
    log.debug("Actor opened" if open else "Actor closed")
    self.actor_moves += 1

Es wäre natürlich auch wieder nicht falsch, wenn die ganzen Zahlen (23, 26) in irgendwelchen Konstanten liegen würden, einfach damit diese Namen haben und man leichter versteht was diese bewirken.

[yankee42]

Und dann den self.actor_moves auch wieder zurücksetzen?

Generell von der Logik her... Der macht alle 10 Schaltvorgänge ein Cooldown. Selbst wenn die 10 Schaltvorgänge auf einen ganzen Tag verteilt sind. Sollte es nicht eher so sein, dass er maximal 10 Schaltvorgänge/5 Minuten zulässt? Das könnte man so lösen:

class CooldownTracker:
    def __init__(self, max_movements: int = 10, max_seconds: int = 30):
        self.movement_times = [0]*max_movements
        self.max_seconds = max_seconds
        self.counter = 0

    def move(self):
        self.movement_times[self.counter] = time.time()
        self.counter = (self.counter + 1) % len(self.movement_times)

    def is_cooldown_necessary(self):
        return time.time() - self.movement_times[(self.counter + 1) % len(self.movement_times)] < self.max_seconds

[LKuemmel]

Vielen Dank für Dein ausführliches Review, @yankee42 !
Deine Vorschläge waren (wie immer) sehr gut und ich habe sie eingearbeitet.

[yankee42]

Ich habe gerade nochmal darüber nachgedacht, ob es sich noch weiter vereinfachen lässt. Mir ist dies hier eingefallen:

class RateLimiter:
    def __init__(self, max_calls: int, max_seconds: int):
        self.movement_times = [0.0]*max_calls
        self.max_seconds = max_seconds
        self.counter = 0

    def __call__(self, delegate: Callable):
        @functools.wraps(delegate)
        def wrapper(*args, **kwargs):
            now = time.time()
            sleep_needed = self.max_seconds - (now - self.movement_times[self.counter])
            if sleep_needed > 0:
                time.sleep(sleep_needed)
            self.movement_times[self.counter] = time.time()
            self.counter = (self.counter + 1) % len(self.movement_times)
            return delegate(*args, **kwargs)
        return wrapper

Das könnte man dann einfach als decorator verwenden:

@RateLimiter(max_calls=10, max_seconds=300)
def __set_actor(self, open: bool):
    # ...

Das würde dazu führen, dass der RateLimiter alles managt. Wenn mehr als 10 Aufrufe innerhalb der letzten 300 Sekunden waren, dann schläft er solange, bis das Fenster wieder eingehalten wird. Das wäre dann noch etwas sparsamer als die bisherige Variante. So wie es jetzt ist wäre es möglich, dass es 9 Aufrufe kurz hintereinander gibt und dann 295 Sekunden den 10. Aufruf und dann würde er sich sich gleich volle 5 Minuten schlafen legen. Mit der obigen Implementierung würde er nur 5 Sekunden schlafen, weil das dafür, dass das Fenster eingehalten wird reicht. Allerdings kann man dadurch nicht mehr steuern wann der Schlaf durchgeführt wird. Ob das nun ein Problem ist dürfte von weiteren Faktoren abhängen, die ich nicht Überblicke. Wenn es genügt einfach nur das Fenster einzuhalten und das sleep nicht ausgerechnet an der Stelle stört, dann wäre der decorator eine schönere Lösung.

Hier noch Tests dazu:

class TestRateLimiter:
    rate_limited_mock: Callable
    mock: Mock
    time: float

    @pytest.fixture(autouse=True)
    def setup(self, monkeypatch):
        self.mock = Mock(return_value="some return value")
        self.rate_limited_mock = RateLimiter(max_calls=2, max_seconds=1)(self.mock)
        self.time = 1000.0
        monkeypatch.setattr(time, 'time', lambda: self.time)
        monkeypatch.setattr(time, 'sleep', lambda seconds: self.sleep(seconds))

    def sleep(self, seconds: float):
        self.time += seconds

    def test_rate_limiter_calls_delegate(self):
        # execution
        result = self.rate_limited_mock(1, 2, a="x", b="y")

        # evaluation
        assert result == "some return value"
        assert self.mock.call_count == 1
        self.mock.assert_called_with(1, 2, a="x", b="y")

    @pytest.mark.parametrize(["num_calls", "expected_time_passed"], [
        pytest.param(2, 0, id="no sleep if in initial limit"),
        pytest.param(3, 1, id="one sleep if just above max_calls threshold"),
        pytest.param(4, 1, id="one sleep if one more above max_calls threshold"),
        pytest.param(5, 2, id="two sleeps if twice above max_calls threshold"),
    ])
    def test_rate_limiter_sleeps_as_needed(self, num_calls: int, expected_time_passed: int):
        # execution
        for i in range(num_calls):
            self.rate_limited_mock()

        # evaluation
        assert self.time == 1000.0 + expected_time_passed

    @pytest.mark.parametrize(["sleep_amount", "expected_time_passed"], [
        pytest.param(0.7, 1, id="reduced sleep"),
        pytest.param(1, 1, id="no sleep (exactly enough time has passed)"),
        pytest.param(2, 2, id="no sleep (more time than threshold has passed)"),
    ])
    def test_rate_limiter_reduces_sleep_if_time_passes_naturally(self, sleep_amount: int, expected_time_passed: int):
        # setup - make sure we are at the point where the next call needs sleep:
        for i in range(2):
            self.rate_limited_mock()

        # pass some time:
        self.sleep(sleep_amount)

        # execution
        self.rate_limited_mock()

        # evaluation
        assert self.time == 1000.0 + expected_time_passed

[LKuemmel]

Die Idee ist gut, allerdings würde dann während dem Cooldown der gesamte Daemon pausieren. Das war bisher so, soll aber verbessert werden.
Ich habe es so abgeändert, dass wenn ein Cooldown erforderlich ist, die Wrapperfunktion nicht aufgerufen wird. Es findet dann in der Zeit keine Bewegung des Aktors statt.

[yankee42]

Wieso lambda? Wieso nicht einfach nur return None?

Ist das denn OK, den Aufruf stillschweigend zu ignorieren? Es geht um die Entriegelung bzw. Verriegelung vom Stecker? Wird dann nicht zum Beispiel der Stecker einfach stillschweigend nicht entriegelt und der Nutzer guckt ziemlich dumm aus der Wäsche?

[LKuemmel]

Die Möglichkeit, Nachrichten vom Ladepunkt an den Master zu senden, ist ein eigenes Projekt. Ich beschränke den Umfang dieses PR auf die Logmeldung.

[andig]

@LKuemmel könnte man in dem Zuge nicht auch #1757 mit lösen?

[LKuemmel]

Die Topics für 2.0 sind nun konsistent. Die Topicnamen werden aus dem ChargepointState-Objekt geparst (

openWB/runs/isss.py

Line 62 in 4f46371

for key, value in vars(counter_state).items():

Die Topics für 1.9 werden nicht mehr angepasst.

[andig]

@benderl heisst das jetzt, dass im Nightly die 1p3p Umschaltung auch im "nur Ladepunkt" Modus funktionieren sollte?

[benderl]

Sollte so sein.

[andig]

Ich würde #2400 dann nach Retest zumachen.