Falsche Nutzung von min/max in 88_HMCCUDEV.pm (u.a.)

Begonnen von RichardCZ, 18 März 2020, 13:52:19

Vorheriges Thema - Nächstes Thema

RichardCZ

Zitat von: CoolTux am 21 März 2020, 14:14:51
Spielst Du auf die Abfrage der JSON Wrapper an?

Ja klar. Prinzipiell bin ich ein Freund fehlertoleranter Modulnutzung (gucken was da ist und dabei nicht auf die Schnauze fliegen),
aber a) kann man das schon ein wenig prozeduraler gestalten als so eine if-Kaskade, b) wäre DAS eine wünschenswerte zentrale "Utils" Komponente fürs FHEM mit einer API a la:


use_module_prio({
    wanted   => qw(encode_json decode_json),
    priority => qw(JSON::MaybeXS JSON::XS Cpanel::JSON::XS),
});


zum Bleistift.

Rückgabewert Modulname, der geklappt hat, ansonsten undef. Dann kann das (=jedes!) aufrufende Modul entscheiden, ob es weitermacht oder nicht.
Überdies könnte die sub gleich testen, ob das betreffende Modul tatsächlich "wanted" bereitstellt.

Man könnte es auch etwas generischer/komplizierter machen, aber mit Overengineering wollte ich mich eigentlich zurückhalten.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

CoolTux

Mach mal bitte einen Patch fertig für die 99_Utils.pm und reiche diesen mit etwas Erklärung bei Rudi ein.
Mich interessiert dann besonders wie meine Implementierung dann ausschauen muss. Aber erstmal schauen wir das der Patch an kommt.
Das sind die angesprochenen kleinen Schritte.
Du musst nicht wissen wie es geht! Du musst nur wissen wo es steht, wie es geht.
Support me to buy new test hardware for development: https://www.paypal.com/paypalme/MOldenburg
My FHEM Git: https://git.cooltux.net/FHEM/
Das TuxNet Wiki:
https://www.cooltux.net

KernSani

Bei der Gelegenheit könnte man vielleicht auch gleich noch ein ,,sicheres" decode_json mitliefern, das Fehler abfängt und loggt.


Gesendet von iPhone mit Tapatalk
RasPi: RFXTRX, HM, zigbee2mqtt, mySensors, JeeLink, miLight, squeezbox, Alexa, Siri, ...

RichardCZ

Zitat von: CoolTux am 21 März 2020, 14:44:07
Mach mal bitte einen Patch fertig für die 99_Utils.pm und reiche diesen mit etwas Erklärung bei Rudi ein.
Mich interessiert dann besonders wie meine Implementierung dann ausschauen muss. Aber erstmal schauen wir das der Patch an kommt.
Das sind die angesprochenen kleinen Schritte.

Für einen Patch ist es noch zu früh, aber so sollte der Code in Grundzügen aussehen:

https://gl.petatech.eu/root/HomeBot/snippets/2

Bei mir tut er zumindest. In der Beschreibung zum Snippet gehe ich noch auf UNIVERSAL (für Methoden) oder exists (einfach für Subroutinen) ein, auf der anderen Seite reden wir hier von einer programmier-API, da sollte der Modulmaintainer schon wissen was er anfordert.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

zap

Zitat von: RichardCZ am 21 März 2020, 15:51:06
Für einen Patch ist es noch zu früh, aber so sollte der Code in Grundzügen aussehen:

https://gl.petatech.eu/root/HomeBot/snippets/2

Bei mir tut er zumindest. In der Beschreibung zum Snippet gehe ich noch auf UNIVERSAL (für Methoden) oder exists (einfach für Subroutinen) ein, auf der anderen Seite reden wir hier von einer programmier-API, da sollte der Modulmaintainer schon wissen was er anfordert.

Nice! Übernehme ich gerne.

Zum Thema Perl und modern. Ich möchte jetzt hier keinen Glaubenskrieg der Programmiersprachen-Jünger vom Zaun brechen. In Perl kann man fast alle Features neuerer Sprachen zumindest teilweise simulieren. Das passiert ja auch z.B. beim Thema Objektorientierung, auch wenn es weit vom Komfort von C++ oder Swift entfernt ist. Wünschen würde ich mir z.B. eine eingebaute, durchgängige Typensicherheit (auch bei skalaren Typen)  (ja, auch das kann man simulieren), Templates usw. Gerade die Typensicherheit würde eine Menge Fehler vermeiden.
in vielen Sprachen ist z.B. Min/Max als template Function definiert. Zusammen mit Typensicherheit gibts keine Probleme mehr bei der Verwendung mit Strings, Integer oder whatever.
2xCCU3, Fenster, Rollläden, Themostate, Stromzähler, Steckdosen ...)
Entwicklung: FHEM auf AMD NUC (Ubuntu)
Produktiv inzwischen auf Home Assistant gewechselt.
Maintainer: FULLY, Meteohub, HMCCU, AndroidDB

zap

Zitat von: KernSani am 21 März 2020, 15:07:45
Bei der Gelegenheit könnte man vielleicht auch gleich noch ein ,,sicheres" decode_json mitliefern, das Fehler abfängt und loggt.


Gesendet von iPhone mit Tapatalk

Besser wäre es, das ganze JSON Modul zu entsorgen und neu zu schreiben. Decode_json ist ja nur die Spitze des Eisbergs
Just my 2 ct.
2xCCU3, Fenster, Rollläden, Themostate, Stromzähler, Steckdosen ...)
Entwicklung: FHEM auf AMD NUC (Ubuntu)
Produktiv inzwischen auf Home Assistant gewechselt.
Maintainer: FULLY, Meteohub, HMCCU, AndroidDB

herrmannj

Das hätte ich fertig (siehe JsonMod). Könnte man ne Lib draus machen

RichardCZ

#22
Um zum eigentlichen Thema (min/max) zurückzukehren:

Hätte man jemanden gefragt der sich mit sowas auskennt.  ;) Bzw. wäre dieser jemand da gewesen, dann

https://forum.fhem.de/index.php/topic,109570.0.html
-> siehe Module::CoreList -> siehe https://metacpan.org/pod/List::Util -> siehe min/max/minstr/maxstr

Ganz ohne CPAN, ganz ohne eigenes neues Rad. Mit der richtigen Nomenklatur, mit Testcases, gewartet von 3. Seite ohne dass man einen Finger krumm machen muss.

Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

RichardCZ

Also diese ganze min/max Geschichte ist in FHEM komplett aus dem Ruder gelaufen.

Verwendung von min(

FHEM/98_Dooya.pm:                $updateState = min( 100, $updateState );
FHEM/98_Dooya.pm:            $newState = min( 100, $posRounded );
FHEM/98_Dooya.pm:            $pos = min( 100, $pos );
FHEM/98_Dooya.pm:                $newPos = min( 100, $pos );
FHEM/98_Dooya.pm:                    $newPos = min( 200, $pos + $newPos );

FHEM/42_SYSMON.pm:        my $cnt  = min( $cnt1, $cnt2 );

FHEM/98_feels_like.pm:    my $T_utci = T_UTCI( $T, $H, min( $W, $max_W ), $P, $T_tmrt );

FHEM/95_PostMe.pm:            my $deltas = min( 3600 * $deltah + 60 * $deltam, 86400 );

FHEM/89_FULLY.pm:    my $repeatCommand = min( AttrVal( $name, 'repeatCommand', 0 ), 2 );
FHEM/89_FULLY.pm:    my $ping          = min( AttrVal( $name, 'pingBeforeCmd', 0 ), 2 );
FHEM/89_FULLY.pm:    my $repeatCommand = min( AttrVal( $name, 'repeatCommand', 0 ), 2 );
FHEM/89_FULLY.pm:    my $ping          = min( AttrVal( $name, 'pingBeforeCmd', 0 ), 2 );
FHEM/89_FULLY.pm:    my $waitAfterPing = min( AttrVal( $name, 'waitAfterPing', 0 ), 2 );


-> Alle ausschliesslich numerisch, benutzen aber die String-Variante

Der definiert lieber seine eigene (numerisch):

FHEM/SHC_datafields.pm:sub min($$) {
FHEM/SHC_datafields.pm:    my $len       = min( $length_bits, 8 - $bit );
FHEM/SHC_datafields.pm:        $len  = min( $length_bits - $src_start, 8 );


Ich finde keine einzige Verwendung von min, wo der Aufrufende mit einem alphanumerischen Kontext rechnet.

=> FHEM würde "richtiger" werden, wenn man einfach min die Semantik von minNum geben würde.

Machen wir die Gegenprobe:

FHEM/Color.pm:    my $m = ::minNum( $r, $g, $b );
FHEM/00_THZ.pm:    my $v0min = minNum(
FHEM/98_logProxy.pm:        $to = minNum( $time, $now );
FHEM/94_PWM.pm:    $newpulseAvg2 = sprintf( "%.02f", $newpulseAvg2 / minNum( 2, $cntAvg ) )
FHEM/94_PWM.pm:    $newpulseAvg3 = sprintf( "%.02f", $newpulseAvg3 / minNum( 3, $cntAvg ) )
FHEM/94_PWM.pm:    my $maxPulse     = ( ( int(@a) > 5 ) ? minNum( $a[5], 1.00 ) : 1.00 );
FHEM/95_Astro.pm:        my $n = minNum( $next[0], @next );
FHEM/10_SOMFY.pm:                $updateState = minNum( 100, $updateState );
FHEM/10_SOMFY.pm:            $newState = minNum( 100, $posRounded );
FHEM/10_SOMFY.pm:    $v = minNum( 100, maxNum( 0, $v ) );
FHEM/10_SOMFY.pm:    $v = minNum( 200, maxNum( 0, $v ) );
FHEM/10_SOMFY.pm:            $pos = minNum( 100, $pos );
FHEM/10_SOMFY.pm:                $newPos = minNum( 100, $pos );
FHEM/10_SOMFY.pm:                    $newPos = minNum( 200, $pos + $newPos );
FHEM/RESIDENTStk.pm:      minNum( ReadingsVal( $name, "lastLocationLong", 0 ), $currLong )
FHEM/RESIDENTStk.pm:      minNum( ReadingsVal( $name, "lastLocationLat", 0 ), $currLat )
FHEM/98_apptime.pm:        my $nice  = minNum( 19, keys %prioQueues );
FHEM/93_PWMR.pm:          minNum( $MaxPulse, ( ( $deltaTemp * $factor )**2 ) + $factoroffset )
FHEM/93_PWMR.pm:        $PVal = minNum( 1, sprintf( "%.2f", $PVal ) );
FHEM/93_PWMR.pm:        $IVal = minNum( 1, sprintf( "%.2f", $IVal ) );
FHEM/93_PWMR.pm:        $DVal = minNum( 1, sprintf( "%.2f", $DVal ) );
FHEM/93_PWMR.pm:        $newpulsePID = minNum( $MaxPulse, sprintf( "%.2f", $newpulsePID ) );
FHEM/93_PWMR.pm:        $IVal = minNum( 1, sprintf( "%.4f", $IVal ) );
FHEM/93_PWMR.pm:        $DVal = minNum( 1, sprintf( "%.4f", $DVal ) );
FHEM/93_PWMR.pm:        $newpulsePID = minNum( $MaxPulse, sprintf( "%.2f", $newpulsePID ) );


Ok. Mit anderen Worten, es gibt im ganzen FHEM keinen einzigen min Aufruf, der mit einem alphanumerischen Kontext rechnet. Natürlich nicht, weil landläufig verstehen die Leute unter "Aal" was anders als "lexikalisch minimales Element meines kleinen Lexikons".

Der schnellste Weg das aufzuräumen?

GROB (muss):

* man haut sub min( aus Utils raus.
* man importiert List::Util qw(min) in den main:: Namespace (also z.B. in Utils)

FEIN (nice to have):

* man benennt überall minNum -> min
* man haut minNum aus Utils raus

Tja, hätte man sowas wie "Janitors", beim Linux Kernel, dann könnten die das machen und man müsste nicht die Modulautoren belästigen.
Aber hat man nicht.

Sollte man im kollektiv-demokratischen Prozess hier in FHEM irgendwann zu der Einsicht gelangen, dass die Linux Kernelentwickler nicht
komplett pillepalle sind und dass "Janitors" eine gute Idee sind, dann mache ich gerne solche Aufräumarbeiten.

Da schon die "Macht" eines Moderators hier im Forum nicht das Richtige für mich ist, dann vielleicht die "Macht" eines Müllmanns?

Ok, man müsste dann für solche Maßnahmen natürlich die altbackene Regel des "keiner fasst des anderen Ringelp Module an." lockern,
aber hey: Fortschritt! Vorwärts immer! Rückwärts nimmah!
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

rudolfkoenig

Du uebersiehst vermutlich, dass min/max fuer die Enduser gebaut wurde, fuer Verwendung in at/notify/etc, die meisten koennen eine Zahl nicht von einem String unterscheiden, und sind durch etliche Themen/etc geimpft, bei Zahlen minNum zu verwenden.

Die erwaehnten Probleme mit den Modulen kann man per Fehlermeldung im passenden Forumsbereich regeln.
Fuer eine Semantikaenderung muesste ich schon von der Mehrheit der Entwickler ueberstimmt werden.
Und dass Du eine Mehrheit fuer eine Janitor-Rolle kriegst, das wage ich zu bezweifeln :)

RichardCZ

Zitat von: rudolfkoenig am 10 April 2020, 12:03:01
Du uebersiehst vermutlich, dass min/max fuer die Enduser gebaut wurde, fuer Verwendung in at/notify/etc, die meisten koennen eine Zahl nicht von einem String unterscheiden, und sind durch etliche Themen/etc geimpft, bei Zahlen minNum zu verwenden.

Übersehe ich nicht. Ich gehe einfach davon aus, dass die Statistik weiterhin Gültigkeit hat, also dass repräsentative Umfragen auch repräsentativ sind.
Du weisst schon: kleine Stichprobe -> Rückschluss aufs große Ganze.
Wenn also schon die Entwickler verarscht wurden, gehe ich davon aus, dass das auch bei den Usern passiert ist.

Man kann ja minNum als Fassade für min behalten.

Zitat
Die erwaehnten Probleme mit den Modulen kann man per Fehlermeldung im passenden Forumsbereich regeln.
Fuer eine Semantikaenderung muesste ich schon von der Mehrheit der Entwickler ueberstimmt werden.
Und dass Du eine Mehrheit fuer eine Janitor-Rolle kriegst, das wage ich zu bezweifeln :)

Ich werde natürlich niemanden auf Knien bitten mir gnädig zu gewähren meine Freizeits-Kapazität einzubringen. Ich kann auch genüsslich zuschauen wie Leute auf dem Müllberg leben.

Aber ich finde es schon klasse wie reagiert wird, wenn ich schwarz auf weiss nicht unerhebliche Bugs in nicht unerheblicher Menge aufzeige.

"Hätte man anders & anderswo melden sollen"
"Soweit ist alles in Ordnung"

Kein Wunder, dass FHEM seit Jahrzehnten praktisch bugfrei ist.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

rudolfkoenig

FHEM ist weder Bugfrei, noch perfekt, aber ich versuche erst vorhandene Probleme zu loesen, und nicht selbst welche auszudenken, damit ich die dann auch noch loesen kann.

RichardCZ

Zitat von: rudolfkoenig am 10 April 2020, 13:52:39
FHEM ist weder Bugfrei, noch perfekt, aber ich versuche erst vorhandene Probleme zu loesen, und nicht selbst welche auszudenken, damit ich die dann auch noch loesen kann.

Das ist ja alles redlich, aber ich habe mir die o.g. Probleme nicht ausgedacht, die sind schon da gewesen. Ich zeige nur mit dem Finger drauf.

Aber hey, wenn jetzt jemand

$markise = min (9, 11);

macht und er bekommt 11 zurück, dann definieren wir das einfach als Feature, bzw. raten ihm minNum zu verwenden. Natürlich nach all dem Supportaufwand den wir (pardon: ihr) hatten um diese kontraintuitive Situation geradezubiegen. Immer und immer wieder.

Zitatund sind durch etliche Themen/etc geimpft, bei Zahlen minNum zu verwenden.

Ich frage mich ja mit welchem Aufwand diese Impfung aufrechterhalten wird.

Alles kein Problem Rudolf, stell einfach nur auf die konservative Art sicher, dass die o.g. Bugs im Code verschwinden (also min -> minNum umbenennen). Wenn das wenigstens konsistent ist, kann man dann irgendwann immer noch einen Regex-Replacer drüberjagen.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

Christoph Morrison


viegener

Zitat von: RichardCZ am 10 April 2020, 12:50:24
Ich werde natürlich niemanden auf Knien bitten mir gnädig zu gewähren meine Freizeits-Kapazität einzubringen. Ich kann auch genüsslich zuschauen wie Leute auf dem Müllberg leben.

Aber ich finde es schon klasse wie reagiert wird, wenn ich schwarz auf weiss nicht unerhebliche Bugs in nicht unerheblicher Menge aufzeige.

Kein Wunder, dass FHEM seit Jahrzehnten praktisch bugfrei ist.

Bugfrei ist FHEM sicher nicht...

Ich finde es gut, dass Du Probleme aufzeigst und speziell auch modulübergreifend denkst. Ich finde den Ton aber einfach zu rauh. Ich vermute bei einigen hier könntest Du viel mehr erreichen, wenn die Aussagen weniger aggressiv wären. Bei "leben auf dem Müllberg" als Vergleich zu FHEM-Modulentwicklungen finde ich
mich nicht wieder.

Kein Support über PM - Anfragen gerne im Forum - Damit auch andere profitieren und helfen können