Timerverwaltung - lib für parallele Funktionsaufrufe

[Beta-User]

Hallo zusammen,

bin jüngst mal wieder über das "Problem" gestolpert, sowas wie eine Timerverwaltung in ein Modul einbauen zu "müssen" bzw. zu wollen.
Konkret geht es um RHASSPY (https://forum.fhem.de/index.php/topic,119447.msg1153802.html#msg1153802) und allgemein gesprochen sieht das Problem so aus: Es kann von verschiedenen externen Stellen eine Art "Dialog" mit dem Modul aufgemacht werden. Nach einer gewissen Zeit sollen (sollen=eigene Festlegung) diese Dialoge dann aber auch wieder geschlossen werden.
Ergo muss für jeden möglichen Dialog bzw. Dialogpartner ein separater Timer angelegt werden, der  bei Fortführung des Dialogs dann auch gezielt wieder gelöscht oder verlängert werden kann, und es "muss" möglich sein, immer dieselbe  zentrale Timeout-Routine anzusprechen, die den betreffenden Timer dann auch zielgerichtet wieder löscht, wenn er nicht (mehr) gebraucht wird. Das Problem dabei ist, dass man nicht wissen kann, wie viele der Timer ggf. am Ende parallel laufen können - immer nur einer wäre einfach...

Um sowas zu realisieren, steht an sich InternalTimer zur Verfügung, ausweichen könnte man auch auf (temporäre) at, beide Varianten muss man aber entweder im Modul verwalten (um die Timer v.a. bei Löschen der Device-Instanz "abschießen" zu können)  oder vom Aufruf her so gestalten, dass sie FHEM nicht abschießen, wenn es die aufgerufene Funktion nicht mehr gibt (also in eine Art eval/AnalyzePerlCommand einpacken).

Im Moment habe ich das nach der "Twilight-" (bzw. "WeekdayTimer"-) "Methode" gelöst und , der Aufruf der Funktion erfolgt über InternalTimer, aber statt wie sonst bei InternalTimer üblich "$hash" zu übergeben, wird ein Hash übergeben, der ua. dann auch $hash enthält, um überhaupt wieder festzustellen, wer die Funktion eigentlich aufgerufen hat und einen "Identifier", über den sich feststellen läßt, zu welchem Dialogpartner der Timer gehört; der letzte Teil wird im Hash der Modul-Instanz verwaltet, solange das relevant ist. In beiden Modulen sind dafür wrapper enthalten, die dann intern InternalTimer bzw. RemoveInternalTimer aufrufen und die Verwaltungsdaten in den Device-Hash schreiben.

Da mir das Thema jetzt zum wiederholten Mal über den Weg gelaufen ist, und ich insbesondere bisher auch keinen "Dreh" gefunden habe, um die "Timer"-lib von Sidey für diese Art Problemstellung :
- übersehe ich mal wieder eine bereits vorhandene Funktionalität?
- gibt es allgemeineren Bedarf dafür? Im Moment ist diese Logik in Twilight und WeekdayTimer so drin - ggf. könnte es also Sinn machen, den Code auszulagern und es entweder in die "Timer"-lib einzubauen oder gesondert. Da die Verwaltung in der Timer-lib aber irgendwie anders abläuft, klingt das für mich eher nach gesondert, aber vermutlich übersehe ich da was...

Für zielführende Hinweise wäre ich euch verbunden, ansonsten bleibt das jetzt erst mal so, wie es ist...

[rudolfkoenig]

Ich verstehe noch nicht das Problem, was hier geloest werden soll.

[CoolTux]

Schau Dir mal die Utils.pm von HTTPMOD an

https://svn.fhem.de/trac/browser/trunk/fhem/lib/FHEM/HTTPMOD/Utils.pm

Da gibt es einige Hilfsfunktionen bezüglich Timer.

[Beta-User]

Schau Dir mal die Utils.pm von HTTPMOD an

https://svn.fhem.de/trac/browser/trunk/fhem/lib/FHEM/HTTPMOD/Utils.pm

Da gibt es einige Hilfsfunktionen bezüglich Timer.

Das geht schon mal in die richtige Richtung, das "Problem" ist da nur, dass die Namen der Timer starr sind ("update"). Würde statt dieses starren Namens eine Option bestehen, einen eigenen Namen zu übergeben, könnte man das auch für RHASSPY wohl mit diesen Funktionen lösen. (Für WeekdayTimer und Twilight ginge es evtl. auch so schon).

(OT: Fehlt in HTTPMOD nicht eine RenameFn? MAn. müsste es Probleme geben, den $hash der ursprünglichen Instanz zu ermitteln, wenn GetUpdate nach Namensänderung der zugehörigen Instanz aufgerufen wird. Ich habe das aber nicht vertieft, vermutlich übersehe ich was.
Die Routinen in RHASSPY umgehen das Problem, indem gleich in das übergebene Argument auch der Instanz-Hash mit übergeben wird.)

Ich verstehe noch nicht das Problem, was hier geloest werden soll.

Bitte entschuldige, wenn ich das Problem bisher nicht hinreichend klar beschrieben habe.

Folgende Situation:
Wir haben uU. (mind.) zwei "Mikrofone", die je unabhängig voneinander einen Dialog zu einem externen Dienst aufmachen. Das kann mehr oder weniger gleichzeitig geschehen. Der Dienst vergibt dann jeweils eine eigene "SessionId", analysiert die jeweiligen Daten und schickt dann die aufbereiteten Daten in einem JSON-Blob an einen MQTT-Server, auf den dann wieder andere Dienste lauschen, u.A. eben FHEM mit Hilfe des Moduls RHASSPY. Die SessionId gilt nur für den Zeitraum zwischen Start eines Dialogs und dessen Beendigung, der Dialog selbst kann aber von allen Teilnehmern am MQTT-Verkehr (eine zeitlang) offen gehalten werden.
Ergo kann es in unserem Beispiel mit den zwei "Mikrofonen" dazu kommen, dass eine RHASSPY-Instanz parallel mit zwei SessionId's umgehen muss, um z.B. von beiden Mikrofonen zu erfahren, welche genaue Anweisung geben werden soll (weil mehrere passen, aber mit einiger Wahrscheinlichkeit nur eine ausgeführt werden soll, oder eine Anweisung z.B. nur ausgeführt werden soll, wenn das ausdrücklich bestätigt wird).

Kommt dann aber weder eine Bestätigung noch eine Auswahl, muss "jemand" irgendwann den Dialog wieder zu machen (und ggf. die Grundeinstellungen für Dialoge mit diesem "Mikrofon" wieder herstellen). Im Moment wird das dadurch erledigt, dass eben für jeden Dialog ein eigener InternalTimer mit eigener "Kennung" angelegt wird.

Die Alternative, auf die ich jetzt wärend des Nachdenkens über das Thema gekommen bin, wäre ggf. die, im Modul ein Array mit den SessionId's (und den zugehörigen Ablaufdaten) vorzuhalten, und dann eben anhand der Ablaufdaten beim Aufruf der timeout-Funktion zu checken, welche SessionId's bereits abgelaufen sind bzw. wann dann der nächste Aufruf erfolgen soll (ähnlich structure_asyncQueue), falls noch was aussteht.

Hätte den Vorteil, dass das zentrale Timer-Array kleiner gehalten wird, aber den Nachteil, dass man beim Setzen eines neuen Timers im Modul dann aufpassen muss, dass man keinen bereits laufenden (mit früherem Endezeitpunkt) löscht. Hat halt alles seine Vor- und Nachteile...

Hier jedenfalls mal der runtergestrippte Code von RHASSPY, vielleicht trägt das zur weiteren Klärung bei (der ist für sich genommen so nicht lauffähig).

Code Auswählen Erweitern


# Device löschen
sub Undefine {
    my $hash = shift // return;

    deleteAllRegisteredInternalTimer($hash);
    RemoveInternalTimer($hash);

    return;
}

sub RHASSPY_DialogTimeout {
    my $fnHash = shift // return;
    my $hash = $fnHash->{HASH} // $fnHash;
    return if (!defined($hash));

    my $identiy = $fnHash->{MODIFIER};

    my $data     = shift // $hash->{helper}{'.delayed'}->{$identiy};
    delete $hash->{helper}{'.delayed'}{$identiy};
    deleteSingleRegisteredInternalTimer($identiy, $hash);

    my $siteId = $data->{siteId};
    my $toDisable = defined $data{'.ENABLED'} ? $data->{'.ENABLED'} : [qw(ConfirmAction CancelAction)];

    my $response = $hash->{helper}{lng}->{responses}->{DefaultConfirmationTimeout};
    respond ($hash, $data->{requestType}, $data->{sessionId}, $siteId, $response);
    configure_DialogManager($hash, $siteId, $toDisable, 'false');

    return;
}

sub setDialogTimeout {
    my $hash     = shift // return;
    my $data     = shift // return; # $hash->{helper}{'.delayed'};
    my $timeout  = shift;
    my $response = shift;
    my $toEnable = shift // [qw(ConfirmAction CancelAction)];

    my $siteId = $data->{siteId};
    $data->{'.ENABLED'} = $toEnable;
    my $identiy = qq($data->{sessionId});

    $response = $hash->{helper}{lng}->{responses}->{DefaultConfirmationReceived} if $response eq 'default';
    $hash->{helper}{'.delayed'}{$identiy} = $data;

    resetRegisteredInternalTimer( $identiy, time + $timeout, \&RHASSPY_DialogTimeout, $hash, 0);
    #InternalTimer(time + $timeout, \&RHASSPY_DialogTimeout, $hash, 0);

#[...]
    return; # $toTrigger;
}


# Eingehende "GetNumeric" Intents bearbeiten
sub handleIntentGetNumeric {
    my $hash = shift // return;
    my $data = shift // return;
    my $value;

    Log3($hash->{NAME}, 5, "handleIntentGetNumeric called");

    # Mindestens Type oder Device muss existieren
    return respond ($hash, $data->{requestType}, $data->{sessionId}, $data->{siteId}, getResponse($hash, 'DefaultError')) if !exists $data->{Type} && !exists $data->{Device};

    my $type = $data->{Type};
    my $subType = $data->{subType} // $type;
    my $room = getRoomName($hash, $data);

    # Get suitable device
    my $device = exists $data->{Device}
        ? getDeviceByName($hash, $room, $data->{Device})
        : getDeviceByIntentAndType($hash, $room, 'GetNumeric', $type)
        // return respond($hash, $data->{requestType}, $data->{sessionId}, $data->{siteId}, getResponse($hash, 'NoDeviceFound'));

    #more than one device
    if (ref $device eq 'ARRAY') {
        #until now: only extended test code
        my $first = $device->[0];
        my $response = $device->[1];
        my $all = $device->[2];
        my $choice = $device->[3];
        my $toActivate = $choice eq 'RequestChoiceDevice' ? [qw(ChoiceDevice CancelAction)] : [qw(ChoiceRoom CancelAction)];
        $device = $first;
        Log3($hash->{NAME}, 5, "More than one device possible, response is $response, first is $first, all are $all, type is $choice");
        return setDialogTimeout($hash, $data, 20, $response, $toActivate);
    }

#[...]

    # Antwort senden
    return; # respond ($hash, $data->{requestType}, $data->{sessionId}, $data->{siteId}, $response);
}


sub handleIntentCancelAction {
    my $hash = shift // return;
    my $data = shift // return;

    Log3($hash->{NAME}, 5, 'handleIntentCancelAction called');

    my $toDisable = defined $data->{customData} && defined $data->{customData}->{'.ENABLED'} ? $data->{customData}->{'.ENABLED'} : [qw(ConfirmAction CancelAction)];
   
    my $response = $hash->{helper}{lng}->{responses}->{ 'SilentCancelConfirmation' };

    return respond ($hash, $data->{requestType}, $data->{sessionId}, $data->{siteId}, $response) if !defined $data->{customData};

    #might lead to problems, if there's more than one timeout running...
    #RemoveInternalTimer( $hash, \&RHASSPY_DialogTimeout );
    my $identiy = qq($data->{sessionId});
    deleteSingleRegisteredInternalTimer($identiy, $hash);
    $response = $hash->{helper}{lng}->{responses}->{ 'DefaultCancelConfirmation' };
    configure_DialogManager($hash, $data->{siteId}, $toDisable, 'false');

    return $hash->{NAME};
}


sub handleIntentConfirmAction {
    my $hash = shift // return;
    my $data = shift // return;

    Log3($hash->{NAME}, 5, 'handleIntentConfirmAction called');

    #cancellation case
    #return RHASSPY_DialogTimeout($hash, 1, $data) if $data->{Mode} ne 'OK';
    return handleIntentCancelAction($hash, $data) if $data->{Mode} ne 'OK';
       
    #confirmed case
    my $identiy = qq($data->{sessionId});
    my $data_saved = $hash->{helper}{'.delayed'}->{$identiy};
    delete $hash->{helper}{'.delayed'}{$identiy};
    deleteSingleRegisteredInternalTimer($identiy, $hash);
   
    my $data_old = $data_saved;

    my $toDisable = defined $data_old && defined $data_old->{'.ENABLED'} ? $data_old->{'.ENABLED'} : [qw(ConfirmAction CancelAction)];
    configure_DialogManager($hash, $data->{siteId}, $toDisable, 'false');

    return respond( $hash, $data->{requestType}, $data->{sessionId}, $data->{siteId}, getResponse( $hash, 'DefaultConfirmationNoOutstanding' ) ) if ! defined $data_old;

    $data_old->{siteId} = $data->{siteId};
    $data_old->{sessionId} = $data->{sessionId};
    $data_old->{requestType} = $data->{requestType};
    $data_old->{Confirmation} = 1;

    my $intent = $data_old->{intent};
    my $device = $hash->{NAME};

    # Passenden Intent-Handler aufrufen
    if (ref $dispatchFns->{$intent} eq 'CODE') {
        $device = $dispatchFns->{$intent}->($hash, $data_old);
    }

    return $device;
}


sub handleIntentChoiceDevice {
    my $hash = shift // return;
    my $data = shift // return;

    Log3($hash->{NAME}, 5, 'handleIntentChoiceDevice called');

    #my $data_old = $data->{customData};
    my $identiy = qq($data->{sessionId});
    my $data_old = $hash->{helper}{'.delayed'}->{$identiy};
    delete $hash->{helper}{'.delayed'}{$identiy};
    deleteSingleRegisteredInternalTimer($identiy, $hash);

    return respond( $hash, $data->{requestType}, $data->{sessionId}, $data->{siteId}, getResponse( $hash, 'DefaultChoiceNoOutstanding' ) ) if ! defined $data_old;

    $data_old->{siteId} = $data->{siteId};
    $data_old->{sessionId} = $data->{sessionId};
    $data_old->{requestType} = $data->{requestType};
    $data_old->{Device} = $data->{Device};

    my $intent = $data_old->{intent};
    my $device = $hash->{NAME};

    # Passenden Intent-Handler aufrufen
    if (ref $dispatchFns->{$intent} eq 'CODE') {
        $device = $dispatchFns->{$intent}->($hash, $data_old);
    }

    return $device;
}




# borrowed from WeekdayTimer
################################################################################
sub resetRegisteredInternalTimer {
    my ( $modifier, $tim, $callback, $hash, $initFlag ) = @_;
    deleteSingleRegisteredInternalTimer( $modifier, $hash, $callback );
    return setRegisteredInternalTimer ( $modifier, $tim, $callback, $hash, $initFlag );
}

################################################################################
sub setRegisteredInternalTimer {
    my $modifier = shift // return;
    my $tim      = shift // return;
    my $callback = shift // return;
    my $hash     = shift // return;
    my $initFlag = shift // 0;

    my $timerName = "$hash->{NAME}_$modifier";
    my $fnHash     = {
        HASH     => $hash,
        NAME     => $timerName,
        MODIFIER => $modifier
    };
    if ( defined( $hash->{TIMER}{$timerName} ) ) {
        Log3( $hash, 1, "[$hash->{NAME}] possible overwriting of timer $timerName - please delete it first" );
        stacktrace();
    }
    else {
        $hash->{TIMER}{$timerName} = $fnHash;
    }

    Log3( $hash, 5, "[$hash->{NAME}] setting  Timer: $timerName " . FmtDateTime($tim) );
    InternalTimer( $tim, $callback, $fnHash, $initFlag );
    return $fnHash;
}

################################################################################
sub deleteSingleRegisteredInternalTimer {
    my $modifier = shift;
    my $hash     = shift // return;
    my $callback = shift;

    my $timerName = "$hash->{NAME}_$modifier";
    my $fnHash    = $hash->{TIMER}{$timerName};
    if ( defined($fnHash) ) {
        Log3( $hash, 5, "[$hash->{NAME}] removing Timer: $timerName" );
        RemoveInternalTimer($fnHash);
        delete $hash->{TIMER}{$timerName};
    }
    return;
}

################################################################################
sub deleteAllRegisteredInternalTimer {
    my $hash = shift // return;
       
    for my $key ( keys %{ $hash->{TIMER} } ) {
        deleteSingleRegisteredInternalTimer( $hash->{TIMER}{$key}{MODIFIER}, $hash );
    }
    return;
}


1;

__END__



[CoolTux]

Das geht schon mal in die richtige Richtung, das "Problem" ist da nur, dass die Namen der Timer starr sind ("update"). Würde statt dieses starren Namens eine Option bestehen, einen eigenen Namen zu übergeben, könnte man das auch für RHASSPY wohl mit diesen Funktionen lösen. (Für WeekdayTimer und Twilight ginge es evtl. auch so schon).

Ich habe den Stefan mal zu dieser Diskussion eingeladen. Mal schauen ob er kommt.

[rudolfkoenig]

@Beta-User: ich verstehe immer noch nicht das Problem.
Ist deine Befuerchtung, dass viele InternalTimer zu "teuer" sind? Ich meine nicht, jedenfalls seit noansi darauf gedraengt hat, sie zu optimieren. Wenn doch, dann bin ich dran.
Oder ist der Aufruf zu kompliziert?
Vermutlich uebersehe ich wieder was.

[Beta-User]

@Beta-User: ich verstehe immer noch nicht das Problem.

Das Problem ist eher die "beschränke" Anzahl an Parametern, die InternalTimer akzeptiert. "Eigentlich" hat man _immer_ $hash, weil man (von einer Modulinstanz aus gesprochen) diesen als Minumum braucht, um überhaupt wissen zu können, um "was es geht". Das ist aber in manchen Fällen zu wenig, also muss man die weitere Info irgendwie "einpacken". Aus "historischen Gründen" erfolgt das bei den "Twilight"-Derivaten über einen Hash, aber evtl. ist ja das Problem, dass ich bisher nicht auf den Gedanken gekommen war, dafür ein Array zu verwenden und das als Argument zu übergeben (wobei man das Array (?) dann auch irgendwo in der Modul-Instanz zwischenspeichern muss, um erforderlichenfalls genau diesen Timer löschen zu können (spätestens in der DeleteFn); ich unterstelle dabei, dass man die ursprünglichen $arg dabei passgenau übergeben muss).

Ist deine Befuerchtung, dass viele InternalTimer zu "teuer" sind?

Na ja, wenn es richtig viele wären: vielleicht. Hier ist die Zahl ziemlich begrenzt, und selbst "ausufernde" WeekdayTimer werden kaum über 20 Schaltpunkte am Tag kommen...
Ergo: Die paar ggf. überflüssige Timer sind nicht mein Schmerz.

Oder ist der Aufruf zu kompliziert?

Eher im Gegenteil, s.o..
Was ggf. etwas umständlich ist, ist ein "renew", also ein erneutes Setzen mit neuer Zeit, das braucht afaik erst ein Remove und dann ein Setzen - mehr oder weniger mit identischen Parametern. Dafür hätte ich die Diskussion aber nicht angefangen...

Vermutlich uebersehe ich wieder was.

Kann ich nicht beurteilen.
Ich meine, in manchen (seltenen!) Fällen fehlt was, aber zum einen gibt's workarounds, und zum anderen sollte man im (vermeintlichen) Bedarfsfall kritisch prüfen, ob man sowas wirklich braucht. Im RandomTimer war nämlich auch diese Art der Timer-Verwaltung aus Twilight auch drin, dort aber völlig unnötig... (es gab noch ein paar "community"-Module, die wegen Umbenennens von "myInternalTimer" auf die Nase gefallen waren; da würde ich auch die Wahrscheinlichkeit auf >90% tippen, dass es dort unnötig kompliziert gelöst war). Das ist mir aber auch erst nach etwas längerer Diskussion mit Sidey über das Thema aufgegangen.

Hier in diesem Thread ging es darum rauszufinden, ob
- ich irgendwas übersehe, und man den gefundenen workaround gar nicht braucht, oder, falls ja, ob er
- ein "akzeptabler" ist oder ggf. sogar zum "Nachbauen" taugt (wenn man wirklich (!) sowas braucht).

(@CoolTux und ggf. StefanStrobel:
Vermutlich bin ich mit der Aussage zu kurz gesprungen, dass das aus der Utils.pm ggf. genutzt werden kann; das "Problem" ist, dass die HTTPMOD-"Familie" bestimmte "Kenner" im Modul-Instanz-Hash abgelegt erwartet, die so bei WDT oder Twilight keinen Sinn ergeben; es geht da nicht um periodisch wiederkehrende Abfragen, sondern um im Voraus berechnete Zeitpunkte, die sich auf den einzelnen Tag beziehen. Die Logik in der Ermittlung des nächsten Zeitpunkts ist also ziemlich anders.)

[herrmannj]

glaub das ist zu kompliziert gedacht. Dies geht doch beliebig (erzeugt beliebig viele an der $id unterscheidbare Timer, $id zb global um das Konzept zu illustrieren):

Code Auswählen Erweitern

my $p = {
  'instance' => $hash,
  'foo' => 'bar',
  'id' => $id++,
...
InternalTimer($ts, \&_doTimer, $p);
}


Code Auswählen Erweitern

sub _doTimer {
  my ($p) = @_;
  my $hash = $p->{'instance'};
  my $foo = $p->{'foo'};
  my $id = $p->{'id'};
  ...
}

[Beta-User]

glaub das ist zu kompliziert gedacht. Dies geht doch beliebig (erzeugt beliebig viele an der $id unterscheidbare Timer, $id zb global um das Konzept zu illustrieren):

Das Erzeugen ist weniger das Problem, meine Sorgen betreffen eher das "Abräumen" (wenn der Timer nicht mehr benötigt wird).

Die "Twilight"-Methode ist genau die, die Timer so anzulegen, nur dass eben $p noch unter $hash->{TIMER}{$id} gepuffert wird, um ihn wieder löschen zu können.
Oder ist meine Sorge wegen des sauberen Wiederabräumens unbegründet?

[Thorsten Pferdekaemper]

Hi,
ich glaube, dass es ganz nett wäre, wenn wir Timer hätten, die in etwa so wie setTimeout / clearTimeout in JavaScript funktionieren.
Also in etwa so:

Code Auswählen Erweitern


my $timerHandle = setTimer($timeStamp, \&myFunc, $arg1, $arg2, $arg3,...);

clearTimer($timerHandle);

Das kann man sich natürlich auch selbst basteln...
Gruß,
   Thorsten

[CoolTux]

glaub das ist zu kompliziert gedacht. Dies geht doch beliebig (erzeugt beliebig viele an der $id unterscheidbare Timer, $id zb global um das Konzept zu illustrieren):

Code Auswählen Erweitern

my $p = {
  'instance' => $hash,
  'foo' => 'bar',
  'id' => $id++,
...
InternalTimer($ts, \&_doTimer, $p);
}


Code Auswählen Erweitern

sub _doTimer {
  my ($p) = @_;
  my $hash = $p->{'instance'};
  my $foo = $p->{'foo'};
  my $id = $p->{'id'};
  ...
}

bei ASC habe ich das erkennen an den funcHash gebunden. Sowohl für Sunrise und Sunset Fahrten als auch die SelfDefense Fahrten habe ich eigene TimerHashs welche entsprechend an das Rolloobject gebunden werden.

[herrmannj]

Das Erzeugen ist weniger das Problem, meine Sorgen betreffen eher das "Abräumen" (wenn der Timer nicht mehr benötigt wird).

Die "Twilight"-Methode ist genau die, die Timer so anzulegen, nur dass eben $p noch unter $hash->{TIMER}{$id} gepuffert wird, um ihn wieder löschen zu können.
Oder ist meine Sorge wegen des sauberen Wiederabräumens unbegründet?

passt doch. Wenn er nicht mehr benötigt wird musst Du ihn halt "abräumen".

Pass nur auf: klassisches mem leak:

Code Auswählen Erweitern

$hash->{TIMER}->{$id}->{'hash'}->{TIMER}->{$id}->{'hash'}->{TIMER}->{$id}->{'hash'}->{TIMER}->{$id}->{'hash'} ...

[herrmannj]

bei ASC habe ich das erkennen an den funcHash gebunden. Sowohl für Sunrise und Sunset Fahrten als auch die SelfDefense Fahrten habe ich eigene TimerHashs welche entsprechend an das Rolloobject gebunden werden.

ja, mach ich auch manchmal mit einer closure (lexical sub). Muss man beim abräumen aber aufpassen

Code Auswählen Erweitern

sub foo {
  my ($hash) = @_;
 
  $p = $id;
  my sub _doTimer {
    my ($id) = @_;
    # alle vars schon da
  }
 
  InternalTimer($ts, _doTimer, $p);
}

[rudolfkoenig]

passt doch. Wenn er nicht mehr benötigt wird musst Du ihn halt "abräumen".

Oder abwarten dass die Funktion aufgerufen wird, und dann nichts machen.
Ob was zu machen ist oder nicht, kann man im Hash ablegen.

my $timerHandle = setTimer($timeStamp, \&myFunc, $arg1, $arg2, $arg3,...);
clearTimer($timerHandle);

Z.Zt. muss man das so schreiben:

Code Auswählen Erweitern

my $timerHandle = [$arg1, $arg2, $arg3,...];
InternalTimer($timesSamp, \&myFunc, $timerHandle);
RemoveInternalTimer($timerHandle);

[Thorsten Pferdekaemper]

Z.Zt. muss man das so schreiben:

Code Auswählen Erweitern

my $timerHandle = [$arg1, $arg2, $arg3,...];
InternalTimer($timesSamp, \&myFunc, $timerHandle);
RemoveInternalTimer($timerHandle);

Das ist nicht ganz dasselbe. Es funktioniert nur, wenn die Argumente immer unterschiedlich sind. Es kann aber gut sein, dass man dieselbe Funktion mit denselben Argumenten einmal in 10s aufrufen will und dann nochmal in 20s.
Natürlich kann man auch das wieder "reparieren"...
Gruß,
   Thorsten 

[Beta-User]

passt doch. Wenn er nicht mehr benötigt wird musst Du ihn halt "abräumen".

Soweit erkennbar, funktioniert das.

Pass nur auf: klassisches mem leak:

Code Auswählen Erweitern

$hash->{TIMER}->{$id}->{'hash'}->{TIMER}->{$id}->{'hash'}->{TIMER}->{$id}->{'hash'}->{TIMER}->{$id}->{'hash'} ...

Darüber muss ich wohl noch eine Weile nachdenken. Wenn es ein akutes Thema ist, ist das "schon immer" ein Problem; nach meinem bisherigen Verständnis wird aber nicht $hash komplett da reingeschrieben, sondern nur die Referenz auf $hash, was unschädlch sein sollte.

(Konkret: der Schnippsel ab hier:)

Code Auswählen Erweitern


sub setRegisteredInternalTimer {
[...]


bei ASC habe ich das erkennen an den funcHash gebunden. Sowohl für Sunrise und Sunset Fahrten als auch die SelfDefense Fahrten habe ich eigene TimerHashs welche entsprechend an das Rolloobject gebunden werden.

Das ist dann dasselbe Prinzip, soweit erkennbar (der Modulcode ist für jemanden Ungeübten wie mich praktisch nicht mehr zu überblicken); nur dass eben der Hash woanders "aufgehoben" wird.

Hi,
ich glaube, dass es ganz nett wäre, wenn wir Timer hätten, die in etwa so wie setTimeout / clearTimeout in JavaScript funktionieren.
Also in etwa so:

Code Auswählen Erweitern


my $timerHandle = setTimer($timeStamp, \&myFunc, $arg1, $arg2, $arg3,...);

clearTimer($timerHandle);

Das kann man sich natürlich auch selbst basteln...
Gruß,
   Thorsten

Na ja, es ging grade drum, das nicht umbedingt immer wieder selbst basteln zu müssen (das geht ja "immer"), sondern eben um einen Weg, der (u.a. memory-leak-) sicher ist...

Für's erste täte es mir genau ein weiteres Argument bei InternalTimer, beliebig viele müssen es ja gar nicht sein, das macht es im Zweifel nur unnötig kompliziert...


Mit anonymen subs habe ich auch schon rumexperimentiert, ala (Pseudocode):

Code Auswählen Erweitern


  InternalTimer($ts, sub _doTimer->($hash,'arg1'), $hash);
}
Das war mir aber zu suspekt für irgendwelche unangenehmen Nebenwirkungen und - soweit ich mich entsinne - kann man in fhemdebug timerList auch nicht mehr erkennen, wer für sowas verantwortlich ist.

Oder abwarten dass die Funktion aufgerufen wird, und dann nichts machen.

Ja, was aber voraussetzt, dass die Funktion noch gefunden wird zum Aufrufzeitpunkt. Ich meine damit mal "Probleme" gehabt zu haben, wenn die letzte Modulinstanz vor Timer-Ablauf gelöscht wurde. (Probleme = kompletter FHEM-Crash im Testsystem...)

Ob was zu machen ist oder nicht, kann man im Hash ablegen.

Klar, sobald man eine "erweiterte Datenstrktur" übergibt (egal, ob Hash oder Array), kann man alles mögliche im Folgecode entscheiden, der muss nur wieder gefunden werden bzw. noch geladen sein (und mit den Argumenten umgehen können).

In Array statt Hash sehe ich jetzt für meinen Anwendungsfall keinen wirklichen Vorteil, oder übersehe ich was (v.a. in Bezug auf das meory leak-Thema)?

[Thorsten Pferdekaemper]

Wenn es ein akutes Thema ist, ist das "schon immer" ein Problem; nach meinem bisherigen Verständnis wird aber nicht $hash komplett da reingeschrieben, sondern nur die Referenz auf $hash, was unschädlch sein sollte.

$hash ist ja die Referenz, sonst wär's %hash...
Memory Leaks entstehen ja gerade dadurch, dass man zyklische Referenzen hat. Dann kann das der Garbage Collector nicht mehr wegräumen und es bleibt ewig stehen. Abhilfe schafft bleiben lassen oder
https://perldoc.perl.org/Scalar::Util#weaken

Na ja, es ging grade drum, das nicht umbedingt immer wieder selbst basteln zu müssen (das geht ja "immer"), sondern eben um einen Weg, der (u.a. memory-leak-) sicher ist...

Ganz meine Meinung...

Gruß,
   Thorsten

[rudolfkoenig]

Das ist nicht ganz dasselbe. Es funktioniert nur, wenn die Argumente immer unterschiedlich sind.

Nope. Es funktioniert, da "my $timerHandle" immer unterschiedlich ist.

Wenn es ein akutes Thema ist, ist das "schon immer" ein Problem;

Ich gehe davon aus, aus dem gleichen Grund muss man bei XML::Dom dispose explizit aufrufen.

Ja, was aber voraussetzt, dass die Funktion noch gefunden wird zum Aufrufzeitpunkt.

Da sowohl Funktion, wie auch Parameter in der Timerverwaltung gemerkt wurden, sollte das kein Problem sein.
Ob die Funktion mit dem Argument noch was anfangen kann, ist eine andere Geschichte.

In Array statt Hash sehe ich jetzt für meinen Anwendungsfall keinen wirklichen Vorteil,

Ich auch nicht, ich wollte die Unterschiede aber minimal halten.

[Thorsten Pferdekaemper]

Nope. Es funktioniert, da "my $timerHandle" immer unterschiedlich ist.

Ja, natürlich... (Gibt es eigentlich kein Hand-an-die-Stirn-Icon?) Es wird ja nachher die Referenz verglichen und nicht deren Inhalt.
Gruß,
   Thorsten

[Beta-User]

Ich gehe davon aus, aus dem gleichen Grund muss man bei XML::Dom dispose explizit aufrufen.

Dann werde ich mir das bei Gelegenheit mal ansehen und auch auf eine "NAME:modifier"-Variante umstellen, die dann $hash wieder aus dem Namen ableitet (und ein return macht, wenn das nicht klappt; eine RenameFn muss dann wohl auch her...).
(Oder hatte ich den Hinweis jetzt falsch verstanden?)

[rudolfkoenig]

(Oder hatte ich den Hinweis jetzt falsch verstanden?)

Nein.
Aber ueblicherweise wird beim TimerAufruf oder Timer Loeschen im globalen Hash der Zeiger auf dem TimerHash entfernt, und damit die Endloskette aufgebrochen. Ein Speicherloch existiert nur, wenn zwei Hashes sich gegenseitig merken, und das Modul keinen Zeiger auf einen der beiden mehr hat.

[Beta-User]

Ok, dann war es vermutlich aus diesem Grund bisher kein (offensichtliches) Problem.

Gibt es Ideen für eine RenameFn?
Wenn $arg vorher "$old_name:$modifier" war, dann muss das Argument neu dann "$new_name:$modifier" sein.
(Ok, dann beantworte mir die Frage gleich selbst: es muss dann eben $time unter dem alten Namen im "TIMER"-Hash gespeichert gewesen sein, dann sollte das klappen, die alten einfach zu löschen und unter neuem Namen anzulegen).

[Beta-User]

es muss dann eben $time unter dem alten Namen im "TIMER"-Hash gespeichert gewesen sein, dann sollte das klappen, die alten einfach zu löschen und unter neuem Namen anzulegen).

Was ggf. noch fehlt, ist die aufzurufende Funktion...

Anbei dann mal ein erster Versuch, das in eine lib zu packen, Doku wäre wohl noch zu ergänzen (wo und wie?)

Eine RenameFn könnte dann so aussehen:

Code Auswählen Erweitern

sub Rename {
    my $new_name = shift // return;
    my $old_name = shift // return;

    my $hash = $defs{$new_name} // return;
    return renameAllRegIntTimer($hash, $new_name, $old_name);
}

Eine aufzurufende Funktion könnte sich so aus dem Sumpf ziehen:

Code Auswählen Erweitern

sub Twilight_fireEvent {
    my $arg = shift // return;

    my ( $name,$modifier ) = split m{:}xms, $arg;
    my $hash = $defs{$name} // return;
[...]


Meinungen dazu?

[rudolfkoenig]

Ich spare mir den Umbennenungsaufwand, indem ich nicht "Name", sondern $defs{Name} merke.
Rename aedert den $hash->{NAME}, und der Rest bleibt gleich.

[Thorsten Pferdekaemper]

Dann werde ich mir das bei Gelegenheit mal ansehen und auch auf eine "NAME:modifier"-Variante umstellen, die dann $hash wieder aus dem Namen ableitet (und ein return macht, wenn das nicht klappt; eine RenameFn muss dann wohl auch her...).
(Oder hatte ich den Hinweis jetzt falsch verstanden?)

Falls das die Maßnahme gegen das Memory-Leak ist, dann wären ggf. Weak References besser. Wie schon angemerkt:
https://perldoc.perl.org/Scalar::Util#weaken
Meiner Meinung nach ist das das "Mittel der Wahl" für zyklische Referenzen.
Gruß,
   Thorsten

[Beta-User]

Irgendwie habe ich bei den beiden letztgenannten Hinweisen noch  gedankliche Hänger, vermutlich weil mein Verständnis dessen, was eine Referenz ist irgendwie noch unvollständig ist:

Ich spare mir den Umbennenungsaufwand, indem ich nicht "Name", sondern $defs{Name} merke.
Rename aedert den $hash->{NAME}, und der Rest bleibt gleich.

Da fehlt mir das konkrete Anwendungsbeispiel.
Wenn ich in die von herrmannj vorgeschlagene HASH-Type Variante nehme, also

Code Auswählen Erweitern

my $p = {
  'instance' => $hash,
  'foo' => 'bar',
  'id' => $id++,
...
InternalTimer($ts, \&_doTimer, $p);
}

ändere zu

Code Auswählen Erweitern

my $p = {
  'instance' => $defs{'instanceName'},
  'foo' => 'bar',
  'id' => $id++,
...
InternalTimer($ts, \&_doTimer, $p);
}
sollte doch eigentlich dasselbe drinstehen, oder? Irgendwie war ich bisher davon ausgegangen, dass $hash (einer FHEM-Device-Instanz) in der FHEM-üblichen Verwendung) dasselbe sei wie $defs{'instanceName'}.
Das scheint aber irgendwie nicht der Fall zu sein.
Oder war es so gemeint, dass der "Text" (z.B.) "HASH(3732AB)" anstatt des Namens verwendet werden soll und dann hinterher einfach quasi automatisch wieder als zur Referenz auf den Hash "erstarkter" Variableninhalt genutzt werden kann ?

Falls das die Maßnahme gegen das Memory-Leak ist, dann wären ggf. Weak References besser. Wie schon angemerkt:
https://perldoc.perl.org/Scalar::Util#weaken
Meiner Meinung nach ist das das "Mittel der Wahl" für zyklische Referenzen.
Gruß,
   Thorsten

Demnach erzeugt das "verhashen" einer Variable keine Kopie, sondern eine Referenz und das hier würde einen memory-leak vermeiden, ohne, dass  die Funktionalität leidet:

Code Auswählen Erweitern


my $instance = $hash;
weaken($instance);
my $p = {
  'instance' => $instance,
  'foo' => 'bar',
  'id' => $id++,
...
InternalTimer($ts, \&_doTimer, $p);
}
Oder habe ich auch da mal wieder was missverstanden oder übersehen...?

[rudolfkoenig]

sollte doch eigentlich dasselbe drinstehen, oder?

Steht ja auch, und eine Aenderung wegen rename ist nicht notwendig.

Wg. weaken: in dem gezeigten Beispiel sehe ich keine zirkulaere Referenz, und keine Notwendigkeit von weaken.
Es waere was Anderes wenn:
- $p _auch_ in $hash eingetragen wird
- beim Aufruf von _doTimer (oder RemoveInternalTimer) $p aus $hash (und umgekehrt $hash aus $p) nicht ausgetragen wird
- $hash spaeter entfernt wird, wieder ohne explizites Entfernen des $p Eintrags.

Anders formuliert:
- perl hat kein Garbage Collection, sondern arbeitet mit Referenz-Zaehler.
- wenn etwas verwendet wird (im hash, Funktionsaufruf, etc), wird der Zaehler erhoeht, und wenn es aus dem Hash entfernt, Ende des Blocks kommt, etc, wird der Zaehler dekrementiert.
- wenn der Zaehler auf 0 dekrementiert wird, dann wird der Speicher freigegeben, und bei allen Unterobjekten der Zaehler dekrementiert.
- bei zirkulaeren Referenzen wird der Zaehler nie auf 0 kommen, deswegen wird es nie freigegeben (ohne kuenstliche Sachen wie weaken, was wiederum ziemlich sicher andere Nebeneffekte hat, sonst waere es nicht optional).

[Thorsten Pferdekaemper]

und das hier würde einen memory-leak vermeiden, ohne, dass  die Funktionalität leidet:

Code Auswählen Erweitern


my $instance = $hash;
weaken($instance);
my $p = {
  'instance' => $instance,
  'foo' => 'bar',
  'id' => $id++,
...
InternalTimer($ts, \&_doTimer, $p);
}

Nicht ganz. Wenn man eine schwache Referenz einer normalen zuweist, dann wird daraus wieder eine starke. D.h. $p->{instance} ist eine normale Referenz.
So geht es richtig:

Code Auswählen Erweitern


my $p = {
  'instance' => $hash,
  'foo' => 'bar',
  'id' => $id++,
...
weaken($p->{instance});
InternalTimer($ts, \&_doTimer, $p);
}
Zusätzlich muss man noch bei der Verwendung abfragen, ob das Teil noch da ist. Bei normalen Referenzen kann man sich darauf verlassen, dass $p->{instance} definiert ist. Bei schwachen Referenzen kann es passieren, dass $p->{instance} "undefiniert" wird. Das passiert, wenn ansonsten keine Referenzen mehr darauf zeigen.
D.h. bei Verwendung muss man immer...

Code Auswählen Erweitern


if(defined($p->{instance})) {
...
}


Wg. weaken: in dem gezeigten Beispiel sehe ich keine zirkulaere Referenz, und keine Notwendigkeit von weaken.

Ich war davon ausgegangen, dass irgendwo sowas steht wie

Code Auswählen Erweitern


$hash->{TIMER} = $p;


- perl hat kein Garbage Collection, sondern arbeitet mit Referenz-Zaehler.

Ich dachte "Garbage Collection" ist der Oberbegriff. Die Müllabfuhr kommt halt dann, wenn man sie bestellt (Reference Counting) und nicht regelmäßig überallhin (Mark and Sweep).

(ohne kuenstliche Sachen wie weaken, was wiederum ziemlich sicher andere Nebeneffekte hat, sonst waere es nicht optional).

Wie meinst Du das? Man kann es ja schlecht nicht-optional machen. Dann würde nämlich keine Referenz jemals was zählen und alles würde sofort wieder weggeworfen werden. Normalerweise will man das ja nicht.
Das Reference Counting in Perl scheint ja so implementiert zu sein, dass sofort alles freigegeben bzw. schwache Referenzen sofort auf undef gesetzt werden. Dadurch merkt man wahrscheinlich relativ schnell, wenn was falsch läuft. Bei Mark and Sweep können schwache Referenzen natürlich lustig werden.

Gruß,
   Thorsten

[rudolfkoenig]

Man kann es ja schlecht nicht-optional machen. Dann würde nämlich keine Referenz jemals was zählen und alles würde sofort wieder weggeworfen werden. Normalerweise will man das ja nicht.

Genau das meinte ich. Man sollte es nicht ohne Nachdenken verwenden.

[Thorsten Pferdekaemper]

Genau das meinte ich. Man sollte es nicht ohne Nachdenken verwenden.

Full ACK.

[Beta-User]

Nicht ganz. Wenn man eine schwache Referenz einer normalen zuweist, dann wird daraus wieder eine starke. D.h. $p->{instance} ist eine normale Referenz.
So geht es richtig:

Danke für den Schnipsel, das hilft mir jetzt wirklich weiter!

Ich war davon ausgegangen, dass irgendwo sowas steht wie

Code Auswählen Erweitern


$hash->{TIMER} = $p;


Ja, klar, sonst kann man die Anforderung nicht umsetzen, die vielleicht jetzt nochmal präziser formuliert so lauten: Es sollen zwei Variablen "irgendwie" an InternalTimer übergeben werden, von denen eine eine eindeutige Zuordnung zu einer bestimmten Modulinstanz ermöglichen, _und_ es soll die Möglichkeit geben, genau diesen Timer wieder zu löschen bzw. ggf. zu erneuern.

Bzgl. RHASSPY war ich grade am Überlegen, ob der Aufwand lohnt, weil kaum jemand häufig sein Device umbenennen wird und es sowieso erforderlich ist, Daten unter der SessionId irgendwo zwischenzuspeichern. Von daher könnte man auch die Namensvariante relativ unproblematisch verwenden und dann eben "nichts" machen, wenn keine Sitzungsdaten vorhanden sind bzw. den Timer unter dem Namen versuchen zu löschen. Geht das (wg. Umbenennung) schief, passiert nichts schlimmes. Das ist bei den anderen Modulen etwas anders, weil da teilweise zyklische Timer dabei sind, die zu einer Reinitialisierung führen => das Modul bleibt schlimmstenfalls stehen. Andererseits sollten sich diese Timer mit schlichtem $hash als Argument anlegen lassen. (Und die Timerverwaltung in Twilight und WeekdayTimer gefällt mir eh' nicht mehr, bei Gelegenheit baue ich das eventuell in eine Art "asyncQueue" um und hinterlege intern, was der nächste Schritt ist.

Werde jetzt jedenfalls im ersten Schritt dann nochmal den vorhandenen Code mit "weaken" aufbohren, und dann mal schauen...

[Beta-User]

Anbei dann erst mal der Code als "lib".

Bin nach der ganzen Diskussion jetzt unschlüssig, ob es sinnvoll ist, den wirklich als lib allg. bereitzustellen, aber zumindest die drei Module gedenke ich jetzt erst mal nach diesem Muster updaten.
Also falls da jetzt doch noch ein "unentdeckter Haken" sein sollte, wäre ich für zielführende Hinweise dankbar.

Das braucht in dieser Form keine RenameFn.

Was der Timer bei Ablauf an Argumenten übergibt, wird in etwa so ausgepackt (die Funktionsnamen in der lib sind etwas anders/gekürzt):

Code Auswählen Erweitern

sub RHASSPY_DialogTimeout {
    my $fnHash = shift // return;
    my $hash = $fnHash->{HASH} // $fnHash;
    return if !defined $hash;

    my $identiy = $fnHash->{MODIFIER};

    my $data     = shift // $hash->{helper}{'.delayed'}->{$identiy};
    delete $hash->{helper}{'.delayed'}{$identiy};
    deleteSingleRegisteredInternalTimer($identiy, $hash);

Timer werden so gesetzt bzw. ggf. erneuert:

Code Auswählen Erweitern

sub setDialogTimeout {
    my $hash     = shift // return;
    my $data     = shift // return; # $hash->{helper}{'.delayed'};
    my $timeout  = shift;
    my $response = shift;
    my $toEnable = shift // [qw(ConfirmAction CancelAction)];

    my $siteId = $data->{siteId};
    $data->{'.ENABLED'} = $toEnable;
    my $identiy = qq($data->{sessionId});

    $response = $hash->{helper}{lng}->{responses}->{DefaultConfirmationReceived} if $response eq 'default';
    $hash->{helper}{'.delayed'}{$identiy} = $data;

    resetRegisteredInternalTimer( $identiy, time + $timeout, \&RHASSPY_DialogTimeout, $hash, 0);
    #InternalTimer(time + $timeout, \&RHASSPY_DialogTimeout, $hash, 0);

#[...]
    return; # $toTrigger;
}

und die UndefFn sieht so aus (unterstellt, es gibt auch noch "normale" Timer):

Code Auswählen Erweitern

sub Undefine {
    my $hash = shift // return;

    deleteAllRegisteredInternalTimer($hash);
    RemoveInternalTimer($hash);

    return;
}

[Thorsten Pferdekaemper]

Hi,
gibt es eigentlich einen Grund, warum überall die Deklaration der Funktionsparameter fehlt? Außerdem werden die Parameter überall per "shift" gefüllt. Das ist alles ziemlich übel unübersichtlich.
Gruß,
   Thorsten

[CoolTux]

Hi,
gibt es eigentlich einen Grund, warum überall die Deklaration der Funktionsparameter fehlt? Außerdem werden die Parameter überall per "shift" gefüllt. Das ist alles ziemlich übel unübersichtlich.
Gruß,
   Thorsten

https://en.wikipedia.org/wiki/Perl_Best_Practices

https://forum.fhem.de/index.php/topic,109526.0.html

[Beta-User]

Hi,
gibt es eigentlich einen Grund, warum überall die Deklaration der Funktionsparameter fehlt? Außerdem werden die Parameter überall per "shift" gefüllt. Das ist alles ziemlich übel unübersichtlich.
Gruß,
   Thorsten

Prototypen (das ist es doch das, was du mit "Deklaration der Funktionsparameter" meinst, oder?) verwende ich seit einiger Zeit nicht mehr. Ist am Anfang etwas gewöhnungsbedürftig, entspricht aber dem, was als "Perl Best Practice" propagiert wird (siehe die Links von CoolTux).

Die Übergabe mit shift hat zwei Vorteile: Zum einen kann man im Fehlerfall bzw. bei nicht genügender Parameter-Übergabe mit "carp" eine gezieltere Fehlermeldung als "prototype mismatch" ausgeben (siehe die "lib"-Variante), zum anderen ist es direkt in der einleitenden Übersicht der Parameter klar erkannbar, was optional ist, was zwingend und man kann auch gleich Ersatzwerte zuweisen.

Ich finde das daher nicht mehr "übel unübersichtlich", sondern hilfreich (selbst wenn es ab ca. 3 Parametern etwas langsamer ist) .

[Thorsten Pferdekaemper]

https://en.wikipedia.org/wiki/Perl_Best_Practices

Nein, ich werde jetzt nicht das Buch kaufen...

https://forum.fhem.de/index.php/topic,109526.0.html

Ach das. Ich dachte, dass sich das geben wird, weil offensichtlich falsch. Er zeigt in dem Beitrag ja gerade, dass er es nicht verstanden hat. Die weitere Argumentation ist dann "weil ich es nicht verstanden habe (also etwas anderes erwarte) muss es weg". Witzig, dass man der Argumentation irgendeine Bedeutung beimisst.
Ich habe noch ein bisschen rumgesucht, ob ich noch bessere Argumente finde. Da war noch was in dem Stil "es wird nicht überprüft und könnte falsch, also kann es weg". Mit der Argumentation dürfte man auch keine Kommentare mehr ins Coding schreiben.

Die Übergabe mit shift hat zwei Vorteile: Zum einen kann man im Fehlerfall bzw. bei nicht genügender Parameter-Übergabe mit "carp" eine gezieltere Fehlermeldung als "prototype mismatch" ausgeben

Das ist aber ein Argument gegen die Prototypen und nicht für das "shift", oder? Dafür ist das Argument tatsächlich valide, würde mir aber nicht ausreichen, um die Unübersichtlichkeit zu kompensieren. Mir ist klares Coding wichtiger als die perfekte Fehlermeldung bei Programmierfehlern. Wenn man den "prototype mismatch" bekommt, dann sieht man sowieso sofort, woran es hängt.

(siehe die "lib"-Variante), zum anderen ist es direkt in der einleitenden Übersicht der Parameter klar erkannbar, was optional ist, was zwingend

Das sehe ich anders, insbesondere, wenn dann plötzlich irgendwo weiter unten mal wieder ein "shift" erscheint. Worauf bezieht sich das dann?
...und wodurch sieht man, was optional ist und was zwingend? Das würde man bei der ($$;$$)-Notation sofort sehen.

Code Auswählen Erweitern


    my ($hash, $data, $timeout, $response, $toEnable ) = @_

Das macht meiner Meinung nach viel klarer, dass das eigentlich die Parameter sind, und darauf kommt es mir an.

...aber vielleicht ist das auch alles ein bisschen Geschmacksache. Mir gefällt der Stil nicht, aber ich will auch meinen Stil niemandem aufzwingen.

Gruß,
   Thorsten

[Beta-User]

Nein, ich werde jetzt nicht das Buch kaufen...

Muss man auch nicht. Man bekommt dieselben Inhalte auch vercoded, z.B. unter http://perlcritic.com/ und kann sich dann da über das jeweilige ? beim "Kritikpunkt" anzeigen lassen, wie man den Punkt ggf. alternativ vercoden könnte.

Ach das. Ich dachte, dass sich das geben wird, weil offensichtlich falsch. Er zeigt in dem Beitrag ja gerade, dass er es nicht verstanden hat. Die weitere Argumentation ist dann "weil ich es nicht verstanden habe (also etwas anderes erwarte) muss es weg". Witzig, dass man der Argumentation irgendeine Bedeutung beimisst.

Man mag über Stilfragen hinsichtlich des gegenseitigen Umgangs streiten, aber afaik ist die Kritik an sich nicht unberechtigt. Ich habe jedenfalls in den übernommenen Modulen einige Beispiele gefunden, bei denen ich _glaube_, dass der ursprüngliche Autor nicht mehr wußte, welche Art der Parameter er wollte und welcher Datentyp überhaupt erwartet wird.

Das ist aber ein Argument gegen die Prototypen und nicht für das "shift", oder?

Jein. Es ist eigentlich eher für "shift", weil das auch dann noch gilt, wenn man mit Prototypen arbeitet

Das sehe ich anders, insbesondere, wenn dann plötzlich irgendwo weiter unten mal wieder ein "shift" erscheint.

Das shift ist an der Stelle nicht mehr unbedingt glücklich, vermutlich wäre es in der Tat besser, das shift gleich zu machen und hinten dann eben eine deutlich längere Prüfung der Randbedingungen vorzunehmen. So ist es eben "direkter" notiert...

...und wodurch sieht man, was optional ist und was zwingend? Das würde man bei der ($$;$$)-Notation sofort sehen.

Das stimmt nun wieder nur bedingt. Es gibt einige Funktionen in FHEM, bei denen der Prototype mit ($$$$) angegeben ist, aber nur der erste Parameter muss wirklich "content" haben, die anderen können "undef" sein, und es gibt auch Fälle, in denen irgendwas zwischendurch "undef" sein kann, aber hinten was gebraucht wird und Fälle, in denen 2 aus 4 genügen. Alle diese Art Abhängigkeiten bekommt man nicht mit, wenn man die vermeintlich übersichtliche Variante

Code Auswählen Erweitern

    my ($hash, $data, $timeout, $response, $toEnable ) = @_

wählt.

"Ganz übel" wird es, wenn es möglich ist, an eine Funktion entweder einen SCALAR oder was anderes (z.B. ein ARRAY) als Argument zu übergeben. Ich nutze das teils, um die weitere Funktionsweise der jeweiligen Funktion umzuschalten.
Bei Verwendung von Prototypen wäre sowas wohl (prinzipbedingt) "verboten" (ob das "guter Stil" ist, ist eine andere Frage, aber da lasse ich mich ggf. gerne beraten, wie es besser geht).

Mir ist klares Coding wichtiger als die perfekte Fehlermeldung bei Programmierfehlern. Wenn man den "prototype mismatch" bekommt, dann sieht man sowieso sofort, woran es hängt.

Mit shift kannst du jedenfalls z.B. auch direkt hinter die jeweilige Zeile einen Kommentar schreiben, welches Argument du erwartest bzw. welche Kombinationen möglich sind. Unter "klarem Coding" versteht halt jeder auch ein klein wenig was anderes, und der "mismatch" ist "nur" ein Inikator für's Zählen oder den erwarteten Datentyp, nicht mehr (aber auch nicht weniger).

...aber vielleicht ist das auch alles ein bisschen Geschmacksache. Mir gefällt der Stil nicht, aber ich will auch meinen Stil niemandem aufzwingen.

Btw.: mir war das am Anfang auch suspekt, und ich bin auch nicht glücklich damit, wie manches von dem, was bestimmte Personen an Inhalten vermitteln wollten, vom Ton her rübergekommen ist. Es ist aber ganz sicher keine reine Einzelmeinung, sonst gäbe es "nur das Buch", und nicht auch noch perlcritic.com bzw. auch die entsprechenden Perl-Packages, mit denen man in etwa dasselbe an Empfehlungen bekommen sollte.
(Und ganz ab von der Prototypen-Geschichte: Mir als Perl-DAM hat das geholfen, noch ganz andere "Klopper" zu finden.)

Heute finde ich das ohne Prototypen und (größtenteils) mit shift persönlich übersichtlicher, mache manches sicher auch noch nicht perfekt und würde z.B. in fhem.pl auch nicht auf den Einsatz von Prototypen verzichten wollen, einfach, weil es zu viele Schnittstellen für Enduser gibt. Und die können in der Tat einiges falsch machen, wenn Funktionen nicht Prototypen-konform aufgerufen werden.
In (gepackagetem) Modulcode sieht das mAn. deutlich anders aus. Da sollte der betr. Modulautor schon wissen, wie er seine Funktionen aufruft.

Aber: Es ist halt anders, und ansonsten bin ich Perl-mäßig eben immer noch eher Lernender.

[Thorsten Pferdekaemper]

Man mag über Stilfragen hinsichtlich des gegenseitigen Umgangs streiten, aber afaik ist die Kritik an sich nicht unberechtigt. Ich habe jedenfalls in den übernommenen Modulen einige Beispiele gefunden, bei denen ich _glaube_, dass der ursprüngliche Autor nicht mehr wußte, welche Art der Parameter er wollte und welcher Datentyp überhaupt erwartet wird.

Das ist genau das nicht-Argument, das ich meine: Irgendjemand versteht irgendwas nicht, deshalb soll man es weglassen. Hä...?
Wenn der Autor nicht mehr weiß, was er da wollte, wie würde es dann helfen, ein Feature wegzulassen, das genau den Punkt, den der Autor nicht versteht, ein bisschen expliziter macht?
Das Problem ist wohl eher, dass der Autor gepennt hat und das nicht ordentlich dokumentiert hat.

Das stimmt nun wieder nur bedingt. Es gibt einige Funktionen in FHEM, bei denen der Prototype mit ($$$$) angegeben ist, aber nur der erste Parameter muss wirklich "content" haben, die anderen können "undef" sein, und es gibt auch Fälle, in denen irgendwas zwischendurch "undef" sein kann, aber hinten was gebraucht wird und Fälle, in denen 2 aus 4 genügen. Alle diese Art Abhängigkeiten bekommt man nicht mit, wenn man die vermeintlich übersichtliche Variante

Code Auswählen Erweitern

    my ($hash, $data, $timeout, $response, $toEnable ) = @_

wählt.

Naja, sowas gehört halt in die Doku der Funktion (wenn es überhaupt wichtig ist). Ich wüsste jetzt nicht, warum man das bei der "shift"-Notation besser sehen sollte. Klar, man kann es ein bisschen kürzer schreiben, aber ich kann ja auch meine Zeile oben hinschreiben und dann danach für die, bei denen es darauf ankommt sowas:

Code Auswählen Erweitern


$hash // croak "hash missing";


"Ganz übel" wird es, wenn es möglich ist, an eine Funktion entweder einen SCALAR oder was anderes (z.B. ein ARRAY) als Argument zu übergeben. Ich nutze das teils, um die weitere Funktionsweise der jeweiligen Funktion umzuschalten.
Bei Verwendung von Prototypen wäre sowas wohl (prinzipbedingt) "verboten" (ob das "guter Stil" ist, ist eine andere Frage, aber da lasse ich mich ggf. gerne beraten, wie es besser geht).

Das finde ich tatsächlich nicht so prickelnd. "Funktionsweise umschalten" klingt doch ganz stark danach, dass es eigentlich zwei Funktionen sein müssten. ...und wenn es denn sein soll, dann kann man an der Stelle einfach Referenzen übergeben und die Unterscheidung mit ref machen. Das ist sowieso sauberer, denke ich.
Und wenn man doch mal eine Funktion machen muss oder will, die mehr Polymorphismus zulassen soll, dann lässt man halt den Prototyp weg. Das bedeutet aber nicht, dass man den Prototypen immer weglassen sollte.

...jetzt muss ich meinen Sohn vom Kindergarten abholen.

Gruß,
   Thorsten

[Beta-User]

Das ist genau das nicht-Argument, das ich meine: Irgendjemand versteht irgendwas nicht, deshalb soll man es weglassen. Hä...?

Na ja, ist m.E. eine Frage der Sichtweise: Wenn man nicht begründen kann, warum man etwas braucht, kann man sagen: haben wir schon immer so gemacht, also brauchen wir es, um uns sicher zu fühlen.
Aber die Sichtweise, einfach alle Dinge wegzulassen, für die es keine zwingende Begründung (mehr) gibt, finde ich auch nicht verkehrt.
Warum sich das Leben an dieser Stelle "künstlich" erschweren, wenn es ganz genauso gut auch ohne diese Restriktion geht...?

Das mit der generellen Tendenz zum Weglassen hat übrigens auch ein klein wenig mit Perl an sich zu tun: Da es sowieso an vielen Stellen nicht wirklich eindeutig ist und vieles erlaubt, von dem andere Programmiersprachen nur "Häh...!?!" verstehen würden, gibt es eigentlich keinen Grund, plötzlich überall eine formale Ebene der Restriktion einzubauen, die die Programmiersprache an sich nicht für _erforderlich_ hält.

Man braucht halt in Perl nicht dieselbe mühselige Aufzählung von möglichen Variablen und Datentypen vorneweg, die man von anderswo gewohnt ist und kann flexibel reagieren, je nachdem, ob man eine Referenz auf einen HASH oder was anderes vorgesetzt bekommt...

(Vermutlich wird bei der einen Funktion, an die ich speziell dachte in der Tat eine Referenz übergeben und dann via ref gecheckt, wie es an einer eher untergeordneten Stelle weitergehen muss).

(Und ich glaube, dass man sowohl dem Buchautor wie dem Thread-Ersteller des von Cooltux zitierten Beitrags durchaus unterstellen darf, dass die sehr genau wissen, was sie - bezogen auf Perl allgemein - tun, und was wirklich erforderlich ist. Das mit dem "nicht-Argument" halte ich daher für sehr kurz gegriffen. Ob speziell das Weglassen von Prototypen im FHEM-Kontext immer passt, ist eine andere Frage, und da hatte ich auch zum pauschalen Weglassen insbesondere in fhem.pl schon was geschrieben).

[CoolTux]

Und genau aus diesen Grund steige ich in solche Diskussionen nicht mehr mit ein. Es endet immer im Endlosen.
Ich schaue mir auch an wie erfahrene Entwickler auf CPAN ihre Module darstellen. Dann gibt es auch noch eine Menge Seiten im Netz wo man dazu was lesen kann.
https://www.perl.com/pub/2005/07/14/bestpractices.html/

Am Ende komme ich für mich persönlich zum Schluss das ich es als besseren Programming Stile empfinde.

Nach Torsten seiner Argumentation sind die ersten 10 Module welche ich spontan auf CPAN finde offensichtlich falsch.

Ach das. Ich dachte, dass sich das geben wird, weil offensichtlich falsch. Er zeigt in dem Beitrag ja gerade, dass er es nicht verstanden hat. Die weitere Argumentation ist dann "weil ich es nicht verstanden habe (also etwas anderes erwarte) muss es weg". Witzig, dass man der Argumentation irgendeine Bedeutung beimisst.

Ich habe für mich selbst entschieden mich an der Mehrheit des Codestils zu halten. Mag sein das Anfänger oder FHEM Perlentwickler ihn schwer lesen können. 90% der anderen Perlentwickler auf der Welt können es aber.


Grüße
Marko

[Thorsten Pferdekaemper]

Wenn man nicht begründen kann, warum man etwas braucht, kann man sagen: haben wir schon immer so gemacht, also brauchen wir es, um uns sicher zu fühlen.

Das "haben wir schon immer so gemacht" hätte ich jetzt für ein Argument gehalten, die Prototypen wegzulassen. Ich hatte bisher den Eindruck, dass die "alten Hasen" hier ein gewisses Beharrungsvermögen haben und deshalb die Prototypen nicht wollen.

Warum sich das Leben an dieser Stelle "künstlich" erschweren, wenn es ganz genauso gut auch ohne diese Restriktion geht...?

Ich sehe das mit den Prototypen nicht als Restriktion. Ich sehe die "Vorschrift", sie wegzulassen eher als solche.

Man braucht halt in Perl nicht dieselbe mühselige Aufzählung von möglichen Variablen und Datentypen vorneweg, die man von anderswo gewohnt ist

Das finde ich auch gut so. Starke Typisierung ist so 80er...

Das mit dem "nicht-Argument" halte ich daher für sehr kurz gegriffen.

Naja, dann sollen die Experten halt mal ein richtiges Argument liefern. Bis dahin verwende ich weiterhin die Prototypen. Wenn andere das nicht wollen, dann ist das auch ok.

Und genau aus diesen Grund steige ich in solche Diskussionen nicht mehr mit ein. Es endet immer im Endlosen.

Ich sehe das nicht so verbissen. Ich rede nur gerne über solches Zeugs. Wer es dann am Ende wie macht ist mir egal. Ich selbst halte mich sowieso nur daran, wenn mich jemand dafür bezahlt.

Nach Torsten seiner Argumentation sind die ersten 10 Module welche ich spontan auf CPAN finde offensichtlich falsch.

Habe ich etwas von "falsch" oder "richtig" gesagt? Es ist ja alles richtig, nur ist manches halt schöner als manches andere.
Ich war immer davon ausgegangen, dass die meisten Perl-Module ziemlich alt sind und deswegen z.B. keine Prototypen haben. Ich wäre nie auf die Idee gekommen, dass das der neue Stil wäre. So ganz glaube ich das auch noch nicht.

Ich habe für mich selbst entschieden mich an der Mehrheit des Codestils zu halten. Mag sein das Anfänger oder FHEM Perlentwickler ihn schwer lesen können. 90% der anderen Perlentwickler auf der Welt können es aber.

Das ist jetzt ein bisschen so wie damals bei VHS, oder? Es ist gut, weil es verbreitet ist....
Tatsächlich ist es beim Programmierstil ein valides Argument, dass es andere auch so machen. Allerdings muss man schon ein bisschen aufpassen, dass man sich damit nicht in den Status Quo eingräbt und Weiterentwicklungen verhindert.

Wie schon gesagt: Ich führe diese "Diskussion" sowieso nur so zum Spaß. Ich bin auch interessiert daran, welche Gründe es für oder gegen bestimmte Entscheidungen gibt. Ich werde deswegen meinen eigenen Stil wahrscheinlich nicht ändern und ich erwarte es auch von niemandem, das zu tun.

Gruß,
   Thorsten

[Beta-User]

Vorab mal:

Ich rede nur gerne über solches Zeugs.

Genau so hatte ich das verstanden: Als Frage, warum ich diese Notation verwende.

Das "haben wir schon immer so gemacht" hätte ich jetzt für ein Argument gehalten, die Prototypen wegzulassen. Ich hatte bisher den Eindruck, dass die "alten Hasen" hier ein gewisses Beharrungsvermögen haben und deshalb die Prototypen nicht wollen.

Na ja, wenn man sich die existierenden Module ansieht, spricht die Statistik eher dafür, dass die "alten Hasen" bei FHEM Prototypen verwenden, und auch (fast) der gesamte Code in der Doku zu FHEM verwendet Prototypen.

Die verlinkte Diskussion geht eher in die Richtung, dass "der Neuling" (der aber mit Perl an sich sehr vertraut ist und daher auch als "alter Hase" bezeichnet werden darf, oder?) klar Stellung gegen Prototypen bezogen hat und erst mal eine eher verhaltene Antworten von jemandem ebenfalls sehr erfahrenen erhalten hat, die in die Richtung ging "Achtung, das hat ggf. Nebenwirkungen, wenn man das einfach so pauschal fordert" (aber nicht: falsch, veraltet oder neumodischer Unfug)...

Ich sehe das mit den Prototypen nicht als Restriktion. Ich sehe die "Vorschrift", sie wegzulassen eher als solche.

Es ging um eine Frage, nicht um eine "Vorschrift". Ich empfinde Prototypen als Restriktion, weil ich hin und wieder halt dann zwischendurch feststelle, dass mir doch noch ein Parameter in der Übergabe fehlt, der zu ergänzen ist, und dann FHEM komplett neu starten muss, wenn die Funktion mit Prototype deklariert war. Neu starten ist im Hauptsystem nicht immer toll.

Naja, dann sollen die Experten halt mal ein richtiges Argument liefern.

Bin kein Experte und habe auch nicht überprüft, ob das wirklich so vorbehaltslos in der Doku steht oder woanders kritisch hinterfragt wird:

Prototypen sind primär dazu gedacht, damit man subs definieren kann die sich wie builtins verhalten. Sagt nicht PBP, sagt die Perl Doku
https://perldoc.perl.org/perlsub.html#Prototypes

Because the intent of this feature is primarily to let you define subroutines that work like built-in functions,

Für modulinterne Funktionen benötige ich in der Regel nicht das feature, dass die sich wie builtins verhalten...

Kann nur nochmal wiederholen, dass ich als Perl-Novize die Erfahrung gemacht habe, dass es ausgesprochen hilfreich war, perlcritic über den Code laufen zu lassen, weil mir damit auch dann diverses anderes Zeug aufgezeigt wurde, was nicht i.O. war (angefangen mit my $foo = 'baz' if $arg;).
Von daher sehe ich es für mich als passend  an, mich schlicht an diese Expertenempfehlungen zu halten, solange ich es nicht wirklich besser weiß.
(Btw.: niemand ist "gezwungen", seinen Code jetzt und sofort zu ändern. Oder den Beispielcode 1:1 zu übernehmen. Man kann den genausogut mit Prototypen verwenden, wenn man das will, und/oder statt shift die "= (@_)"-Variante.)

Hoffe, das jetzt einigermaßen verständlich klargestellt zu haben und danke für die Frage.

[Beta-User]

Zwischenstand, nachdem ich jetzt nochmal intensiver über den WeekdayTimer-Code gebrütet hatte:

Es gibt bestimmte Fälle, in denen es umständlich ist, auf anderem Weg die Timerverwaltung zu realisieren und der Weg über  im Gerätehash registrierte Timer-Hashes der "einfachste" Weg ist, bestimmte Dinge umzusetzen, insbesondere dann auch nachträglich noch weitere Parameter mit in den Timer-Hash zu schreiben (das feature verwendet WDT an mind. einer Stelle).

Ich würde daher darum bitten, dass sich jemand Kompetenteres wie meinereiner nochmal den Vorschlag aus https://forum.fhem.de/index.php/topic,120813.msg1154322.html#msg1154322 zu Gemühte führt, bevor ich den WDT dann als erstes Modul in diese Richtung umbaue.

Die Doku würde ich dann entsprechend dem Muster in "Core::Authentication::Passwords.pm" machen...?

EDIT: aktualisierte Version samt Doku anbei.

[rudolfkoenig]

Ich würde daher darum bitten, dass sich jemand Kompetenteres wie meinereiner nochmal den Vorschlag aus https://forum.fhem.de/index.php/topic,120813.msg1154322.html#msg1154322 zu Gemühte führt, bevor ich den WDT dann als erstes Modul in diese Richtung umbaue.

Ich habe mich angesprochen gefuehlt, und den Beitrag samt Code durchgelesen. Da wird Einiges gemacht, was fuer die Timerverwaltung selbst nicht notwendig ist, und etliche Funktionen sind nicht oder nur unvollstaendig drin, insofern kann ich nicht sagen, ob irgendetwas fehlt. Was grob Falsches habe ich nicht gesehen.
Und ich glaube nicht, dass Du fuer diese Aufgabe jemand "Kompententeres" brauchst.

[Beta-User]

Danke für's Drübersehen und die netten Worte.
Für (schon verarbeitete) Stichworte wie "weaken" wären meine Kenntnisse nicht hinreichend gewesen, von daher frage ich lieber, als mich zu überschätzen...

Klar, es geht eher um's "Drumrum" denn um die eigentliche "Verwaltung" - das kann fhem.pl deutlich besser. Bei der WDT-Überarbeitung habe ich dann aber gesehen, dass da _zwischendurch_ auch noch keys in den Hash geschoben werden, und das fand ich dann wirklich einen interessanten Ansatz, von dem ich zumindest auf die Schnelle keine Idee hätte, wie man das (deutlich ) anders lösen könnte...
(Hoffe, die Doku ist einigermaßen klar dahingehend, dass man sich den Overhead nur antun sollte, wenn man wirklich was in der Art benötigt).