-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update 00_SIGNALduino.pm - fix break & WARNING #592
update 00_SIGNALduino.pm - fix break & WARNING #592
Conversation
- added check method defined on protocolDef - check defined method exist -> not exists, ERROR Message | no method defined, used system general method SIGNALduino_MCRAW --> no SIGNALduino break an PEARL WARNING
FHEM/00_SIGNALduino.pm
Outdated
{ | ||
SIGNALduino_Log3 $name, 5, "$name: Error: Unknown function=$method. Please define it in file $0"; | ||
my $method; | ||
if (!exists $ProtocolListSIGNALduino{$id}{method}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Die Intention verstehe ich, allerdings würde ich es etwas anders lösen.
Ich glaube wir hatten einen vergleichbaren Fall auch schon mal. Ich hoffe ich finde die codestelle.
Die Defaults würde ich 1x beim Einlesen der Daten setzen und nicht bei jeder Verarbeitung.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Die Defaults würde ich 1x beim Einlesen der Daten setzen und nicht bei jeder Verarbeitung.
korrigierst du den code wenn du die Stelle fandest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ja, ich hoffe es eilt nicht zu sehr :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Die Stelle habe ich Gefunden.... allerdings ist das noch in 00_SIGNALduino.pm
Das passt da aber nicht gut hin...
Ihh bin jetzt etwas hin und her gerissen.
- Die Protokolldaten werden über lib::SD_Protocols verwaltet.
Ich könnte dort eine Funktion einbauen die nach LoadHash aufgerufen wird.
In etwa sowas wie setDefaut
. Dann werden die Standardwerte im Hash hinterlegt.
Das Abrufen der Daten würde ich auf getProperty($id,"method");
ändern.
- Die Werte müssen nur einmal gesetzt werden und sind dann immer vorhanden
- 00_SIGNALduino.pm hat keine Kontrolle mehr über die Daten.
- In 00_SIGNALduino.pm wird der Wert von Method via
$method= checkProperty($id,"method",\&main::SIGNALduino_MCRAW);
Gesetzt. Dann wird zur Laufzeit verifiziert ob es den method Wert gibt und dann dieser oder der default zurückgegeben.
- Die Zugriffe auf die Protokolldaten würde ich ohnehin umstellen wollen
- Das prüfen eines Inhaltes zur Laufzeit kostet ein paar CPU Zyklen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was du vorhast, habe ich verstanden. Was durchaus besser oder CPU sparender ist, kann ich schwer einschätzen.
Das ein Standard gesetzt wird, wenn Eintrag noch vorhanden ist, finde ich gut.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ich denke, den Standard kann man beim Laden der Protokolldaten setzen. Das Programm funktioniert ja ohne die Methode nicht.
Deinen PR müsste ich dafür aber ein bisschen erweitern :)
Wie ist es denn bei MU und MS geregelt? Ich finde, es sollte wie postDemodulation behandelt werden, um es zu vereinheitlichen und damit auch vereinfachen. Unter Umständen könnte es ja sogar eine Routine für alle werden. |
Naja, postDemodulation ist optional und muss nicht vorhanden sein. |
Warum ist die Angabe eigentlich nicht optional? Wenn sowieso an den Daten keine Veränderung erforderlich ist, braucht man die "method" auch nicht. |
Ohne die methode gibt es nichts zum Ausgeben. Daher ist eine Protokolldefinition ohne diese methode für nichts zu gebrauchen. |
Mhmm, das verstehe ich jetzt nicht.
|
Wenn du dies musst, kein Problem ;) |
Ich habe es damals so implementiert, da alle MC Protokolle eine indivduelle Verarbeitung benötigt haben. Ich finde die Variante, verschiedene Verarbeitungsoptionen je Protokoll anwenden zu können auch flexibel. |
Gut ich mach das. Hatte jetzt nur ein paar andere Aktivitäten um die Ohren. |
Ich finde es wenig effizient, eine Routine als Standard zu definieren, die eigentlich nichts tut, außer einmal hin und wieder zurück zu wandeln. |
Added sub to set defaults moved MCRAW Output sub to SD_Protocols.pm
removed SIGNALduino_MCRAW
Ich habe das nun mal umgeändert. |
Du kannst es auch so machen, das das Protokoll ignoriert wird ABER dann bitte einen Hinweis für den User mit maximal Verbose 3. Man kann es so oder so auslegen wie man solch einen Fehler abfängt. Wir sollten nur keine Pear Warning generieren. |
Da stimme ich dir zu. |
Ich habe hier den Konlfikt beseitigt.
Du meinst, wenn es die Sub nicht gibt, so wird nichts angestellt? |
Manchmal macht das Handy einfach was es will und das ist nicht was ich gerne hätte. Auf die Schnelle ist mir nicht eingefallen, wie man das überprüft außer via eval. |
Added check for existing sub SD_Protocols.pm fixed sub syntax for args test_SD_Protocols-definition.txt Fixed check for protocol with id 0. Any protocol with a 0 was threated as 0 which is wrong added test if setDefaults does it job to add method if it is not present added test for sub MCRAW. not finished for now test_loadprotohash-definition.txt moved tests to subtest
…r/RFFHEM into dev-r34_method_check
fixed test sub MCRAW() - test output with max length equal current length - test output with current length higher max length
So ich habe den Fehler mit einer falschen sub abgefangen und auch Tests erstellt. |
Sind hier von mir noch Handlungen notwendig weil "At least 1 approving review is required by reviewers" steht aber ich kein approve geben kann? |
Schau doch bitte noch mal drüber. |
@@ -2535,7 +2535,7 @@ SIGNALduino_Parse_MC($$$$@) | |||
SIGNALduino_Log3 $name, 5, "$name: extracted data $bitData (bin)"; | |||
|
|||
my $method = lib::SD_Protocols::getProperty($id,"method"); | |||
if (!exists &$method) | |||
if (!exists &$method || !defined &{ $method }) | |||
{ | |||
SIGNALduino_Log3 $name, 5, "$name: Error: Unknown function=$method. Please define it in file $0"; | |||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ich muss hier nochmal einhaken:
Wenn ich das richtig verstehe, wird bei falscher method oder bei nicht existierender method eine Fehlermeldung nur bei verbose 5 ausgegeben und es erfolgt keine weitere Auswertung. Der User bekommt im Normalfall (verbsoe 3) davon nichts mit.
Warum drehst du das Ganze nicht einfach rum:
Bei existierender method diese anwenden, sonst halt keine anwenden und einfach die Auswertung fortsetzen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existieren wird der Eintrag method immer, solange nicht jemand "rumbastelt".
Die Fehlermeldung, ob die definierte Methode auch existiert ist eher für Entwickler gedacht, welche die Angabe der Methode ja vornehmen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Man könnte den Eintrag aber auch einfach weglassen, wenn man keine Vorverarbeitung braucht, da es sowieso in einem selbst entwickelten Modul weiter verarbeitet wird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Die "Vorverarbeitung" soll prüfen,
- ob die maximale Länge überschritten wurde
- Die Daten von einer binären darstellung in eine hexadezimale wandeln
Ich verstehe nicht, wieso dieses flexiblen Auswerten geändert werden soll.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ich dachte ja nur, das es ein guter Zeitpunkt wäre, das gleich zu optimieren...
Mir ist nichts aufgefallen.
Wenn ich @elektron-bbs richtig verstanden habe, so meint er das so, das wenn keine Methode angegeben ist, das dann die Bits unverändert herauskommen. Es geht nicht ums ändern, es geht darum, das wenn nichts angegeben ist und auch die Methode nicht angegeben wurde, das als Standard benutzt wird. (ich vermute als Endresultat in der Verarbeitungsroutine) Die flexible Verarbeitung ist dadurch bleibend. Sobald du eine separate Verarbeitung benötigst, so kann du diese flexible angeben. |
Ohne Verarbeitungsmethode fehlt die Längenprüfung und es gibt auch keine Daten zum Dispatchen. |
Pull Request Test Coverage Report for Build 1519
💛 - Coveralls |
1 similar comment
Pull Request Test Coverage Report for Build 1519
💛 - Coveralls |
--> no SIGNALduino break an PEARL WARNING
Undefined subroutine &main::SIGNALduino_MCRAW1 called at ./FHEM/00_SIGNALduino.pm line 2546.
to TEST:
a) in file SD_ProtocolData.pm file delete method
b) in file SD_ProtocolData.pm file rename method to a unknown name