elektron-bbs commented 1 year ago

Please check if the PR fulfills these requirements
[x] Tests for the changes have been added / modified (needed for for bug fixes / features)
[x] commandref has been added / updated (needed for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
[ ] Bugfix (please link issue)
[x] Feature enhancement
[ ] Documentation update
[ ] Unittest enhancement
[ ] other
What is the current behavior? (You can also link to an open issue here, if this describes the current behavior) No decoding of remote control commands. https://forum.fhem.de/index.php/topic,53282.msg1240911.html#msg1240911
What is the new behavior (if this is a feature change)? Receive and send these remote control commands.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?) no
Other information:

codecov[bot] commented 1 year ago

Codecov Report

Merging #1129 (7d3da67) into master (1d5733d) will increase coverage by 0.26%. The diff coverage is 47.61%.

@@            Coverage Diff             @@
##           master    #1129      +/-   ##
==========================================
+ Coverage   66.84%   67.11%   +0.26%     
==========================================
  Files         132      135       +3     
  Lines        9785     9806      +21     
  Branches     1570     1572       +2     
==========================================
+ Hits         6541     6581      +40     
+ Misses       1952     1933      -19     
  Partials     1292     1292

Flag	Coverage Δ
fhem	`56.64% <47.61%> (+0.33%)`	:arrow_up:
modules	`67.11% <47.61%> (+0.26%)`	:arrow_up:
perl	`90.33% <ø> (+0.16%)`	:arrow_up:
unittests	`67.11% <47.61%> (+0.26%)`	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
FHEM/lib/SD_ProtocolData.pm	`100.00% <ø> (ø)`
lib/Test2/SIGNALduino/RDmsg.pm	`74.00% <0.00%> (-0.75%)`	:arrow_down:
t/FHEM/14_SD_UT/01_attr.t	`100.00% <ø> (ø)`
t/FHEM/14_SD_UT/03_set.t	`92.30% <ø> (ø)`
FHEM/14_SD_UT.pm	`48.76% <50.00%> (+3.88%)`	:arrow_up:
FHEM/10_SD_Rojaflex.pm	`66.39% <0.00%> (-4.05%)`	:arrow_down:
FHEM/00_SIGNALduino.pm	`63.64% <0.00%> (-0.45%)`	:arrow_down:
t/FHEM/14_SD_AS/00_load.t	`100.00% <0.00%> (ø)`
t/FHEM/14_Hideki/00_load.t	`100.00% <0.00%> (ø)`
t/FHEM/10_SD_Rojaflex/09_parseData.t	`94.11% <0.00%> (ø)`
... and 1 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

elektron-bbs commented 1 year ago

schon wieder codecov/patch :-(

Wieso mäkelt das Teil an diesen Zeilen:

+        ############ RCnoName20 ############
+        } elsif ($attrValue eq 'RCnoName20' || $attrValue eq 'RCnoName20_10' || $attrValue eq 'DC_1961_TG') {
+          $deviceCode = substr($bitData,0,16);
+          $deviceCode = sprintf("%04X", oct( "0b$deviceCode" ) );
+          $devicename = $devicemodel.'_'.$deviceCode;

Testdaten für alle drei Fernbedienungen existieren. Allerdings habe ich die Testdaten für 'DC_1961_TG' zuerst nur im Branch Master vom SD_Tool gehabt und erst nach dem Pull request ein merge in pre-release durchgeführt.

Generelle Frage: Wann sind wo (master, pre-release) Testdaten erforderlich?

sidey79 commented 1 year ago

schon wieder codecov/patch :-(

Wieso mäkelt das Teil an diesen Zeilen:
+        ############ RCnoName20 ############
+        } elsif ($attrValue eq 'RCnoName20' || $attrValue eq 'RCnoName20_10' || $attrValue eq 'DC_1961_TG') {
+          $deviceCode = substr($bitData,0,16);
+          $deviceCode = sprintf("%04X", oct( "0b$deviceCode" ) );
+          $devicename = $devicemodel.'_'.$deviceCode;
Testdaten für alle drei Fernbedienungen existieren. Allerdings habe ich die Testdaten für 'DC_1961_TG' zuerst nur im Branch Master vom SD_Tool gehabt und erst nach dem Pull request ein merge in pre-release durchgeführt.

Generelle Frage: Wann sind wo (master, pre-release) Testdaten erforderlich?

Die Tests werden einmal mit den Daten aus pre-release und einmal mit denen aus dem master ausgeführt. Wenn es Fehler im pre-release gibt, dann wird der Test nicht als fehlerhaft gewertet, das ist eigentlich alles.

Sollten wir die Testdaten zu den Modulen legen, dann wird das irrelevant.

Besagter Abschnitt wird im übrigen nur aufgerufen, wenn schon mal was empfangen wurde: https://github.com/RFD-FHEM/RFFHEM/blob/49766ffc5a9232b7a7d56c730815329c76f63ad3/FHEM/14_SD_UT.pm#L2255

Das ist ja hier nicht der Fall, es wird erst das attribut gesetzt und danach erst parseMsg aufgerufen, daher wird der codeabschnitt nicht angesteuert.

elektron-bbs commented 1 year ago

Generelle Frage: Wann sind wo (master, pre-release) Testdaten erforderlich?

Die Tests werden einmal mit den Daten aus pre-release und einmal mit denen aus dem master ausgeführt. Wenn es Fehler im pre-release gibt, dann wird der Test nicht als fehlerhaft gewertet, das ist eigentlich alles.

Demnach könnten wir uns die Tests mit pre-release eigentlich sparen.

Sollten wir die Testdaten zu den Modulen legen, dann wird das irrelevant.

Mhmm, willst du dann für jedes Modul ein JSON pflegen?

Besagter Abschnitt wird im übrigen nur aufgerufen, wenn schon mal was empfangen wurde:

https://github.com/RFD-FHEM/RFFHEM/blob/49766ffc5a9232b7a7d56c730815329c76f63ad3/FHEM/14_SD_UT.pm#L2255

Das ist ja hier nicht der Fall, es wird erst das attribut gesetzt und danach erst parseMsg aufgerufen, daher wird der codeabschnitt nicht angesteuert.

Stimmt, demnach müsstest du im Test das InternalVal 'bitMSG' vorher befüllen, oder halt die Nachricht zweimal dispatchen.

sidey79 commented 1 year ago

Demnach könnten wir uns die Tests mit pre-release eigentlich sparen.

Sollten wir die Testdaten zu den Modulen legen, dann wird das irrelevant.

Mhmm, willst du dann für jedes Modul ein JSON pflegen?

Das war zumindest mein Verständnis aus https://github.com/RFD-FHEM/SIGNALduino_TOOL/issues/87 Dadurch kann dann auch die Unterscheidung in pre_release entfallen, da die Tests immer passend zum Versionsstand des Modules geladen werden.

Besagter Abschnitt wird im übrigen nur aufgerufen, wenn schon mal was empfangen wurde: https://github.com/RFD-FHEM/RFFHEM/blob/49766ffc5a9232b7a7d56c730815329c76f63ad3/FHEM/14_SD_UT.pm#L2255

Das ist ja hier nicht der Fall, es wird erst das attribut gesetzt und danach erst parseMsg aufgerufen, daher wird der codeabschnitt nicht angesteuert.

Stimmt, demnach müsstest du im Test das InternalVal 'bitMSG' vorher befüllen, oder halt die Nachricht zweimal dispatchen.

Richtig, über prep_commands kann man ein beliebiges Kommando nach Define, aber vor dem Setzen der Attribute ausführen. Ein zweimaliges dispatchen würde nicht helfen, denn das kommt nachdem Attribute gesetzt werden.

elektron-bbs commented 1 year ago

Mir sind noch zwei "return undef" aufgefallen. Kann ich das ändern, oder ist das im Moment eher ungünstig?

sub SD_UT_bin2tristate {
  my $bitData = shift // return undef;

sub SD_UT_tristate2bin {
  my $tsData = shift // return undef;

Testdaten für diesen PR sind z.Z. allerdings nicht im JSON.

sidey79 commented 1 year ago

@actions-user Das kannst Du anpassen, kein Problem.

Testdaten sind hier und werden auch ausgeführt: https://github.com/RFD-FHEM/SIGNALduino_TOOL/commit/cd4600a8337e51039cf3c4ea58e3a0b8a0db0dcc

elektron-bbs commented 1 year ago

Testdaten sind hier und werden auch ausgeführt: RFD-FHEM/SIGNALduino_TOOL@cd4600a

Das war mein commit vom Dec 10, 2022. Die Daten wurden aber wieder gelöscht am Dec 12, 2022 durch diesen https://github.com/RFD-FHEM/SIGNALduino_TOOL/commit/306d2562906bcedbca737f259db3d7f3559e2e99 (remove testdata for remote control DC_1961_TG protocol 20 (later addition)).

sidey79 commented 1 year ago

Testdaten sind hier und werden auch ausgeführt: RFD-FHEM/SIGNALduino_TOOL@cd4600a

Das war mein commit vom Dec 10, 2022. Die Daten wurden aber wieder gelöscht am Dec 12, 2022 durch diesen RFD-FHEM/SIGNALduino_TOOL@306d256 (remove testdata for remote control DC_1961_TG protocol 20 (later addition)).

Im Branch pre-release ist er ja noch enthalten: https://github.com/RFD-FHEM/SIGNALduino_TOOL/commit/e98c1bc199c5396b4adc2d1d733371b56a820e7d

HomeAutoUser commented 1 year ago

Im Branch pre-release ist er ja noch enthalten: RFD-FHEM/SIGNALduino_TOOL@e98c1bc

Das ist nicht richtig @sidey79. Hier https://github.com/RFD-FHEM/SIGNALduino_TOOL/blob/pre_release/FHEM/lib/SD_Device_ProtocolList.json ist nichts mehr drin. Grund, nach deiner Aussage, das es voreilig war diese reinzunehmen, entfernte ich es im Master und Pre-Release.

sidey79 commented 1 year ago

Pre_Release ist ein anderer Branch als pre-release. Letzterer wird für Tests verwendet und da können auch Tests aufgenommen werden die noch nicht im master Branch erfolgreich laufen.

HomeAutoUser commented 1 year ago

Pre_Release ist ein anderer Branch als pre-release. Letzterer wird für Tests verwendet und da können auch Tests aufgenommen werden die noch nicht im master Branch erfolgreich laufen.

Ja es gibt 2 fast gleiche Branchs .... diese sind sehr Different. Wenn ich diesen nun gleich ziehe, weil das Master vom Umfang der Funktionen des TOOL´s voran schritt, so ist Pre_Release ebenso ohne die Daten.

Frage: WIESO testen wir etwas im pre-release wenn die Tests noch nicht fertig sind? WIESO machen wir es nicht gleich richtig? Wenn wir im Pre... immer voraus eilen und im Master etwas rein kommt, weil der Code oder das TOOL entwickelt bzw. verbessert wird, so kommen Konflikte auf.

elektron-bbs commented 1 year ago

Generelle Frage: Wann sind wo (master, pre-release) Testdaten erforderlich?

Die Tests werden einmal mit den Daten aus pre-release und einmal mit denen aus dem master ausgeführt. Wenn es Fehler im pre-release gibt, dann wird der Test nicht als fehlerhaft gewertet, das ist eigentlich alles.

Sollten wir dann nicht pre-release "beerdigen", wenn es es eh nichts auswirft?

sidey79 commented 1 year ago

Sollten wir dann nicht pre-release "beerdigen", wenn es es eh nichts auswirft?

Sobald alle Testdaten bei den Modulen liegen können wir das machen. Jetzt ist es doch praktisch.

elektron-bbs commented 1 year ago

Mir nutzt es aber nichts, wenn ich auf Fehler in den Tests erst dann aufmerksam gemacht werde, wenn ich Test-Daten im Master-Branch hinterlegt habe und vorher denke, es ist alles im grünen Bereich, weil nur mit pre-release getestet wird und das keine Fehler ausgibt. Wenn ich die Test-Daten vorher schon in Master lege, blockiere ich aber damit andere Pull requests.

sidey79 commented 1 year ago

Richtig erkannt. Da ist manuelles nachschauen angesagt, aber das hat sich ja in kürze erledigt

HomeAutoUser commented 1 year ago

Da ist manuelles nachschauen angesagt, aber das hat sich ja in kürze erledigt

Ist das die Aussage um sämtliche Tests fehlerfrei zu gestalten erstmal oder die Zusage um nur den Master Branch zu nutzen ;-) ?

sidey79 commented 1 year ago

Da ist manuelles nachschauen angesagt, aber das hat sich ja in kürze erledigt

Ist das die Aussage um sämtliche Tests fehlerfrei zu gestalten erstmal oder die Zusage um nur den Master Branch zu nutzen ;-) ?

Die Frage irritiert mich. Das mit dem "master" Branch hat sich doch erledigt wenn die Testdaten migriert wurden.

HomeAutoUser commented 1 year ago

Ich denke wir kommunizieren alle etwas "aneinander vorbei machmal" oder jeder von uns interpretiert aufgrund des großen Umfang der Tests etwas anderes.

Können wir nicht Fakten schaffen um nicht aneinander vorbei zu reden? 1) Testdaten SD_UT in Master und die Tests dazu fertig machen 2) zukünftig die Struktur vom neuen Schema JSON die Tests einbringen wie sie für SD_WS vorbereitet sind als Datensatz aber der bisherige Test noch nicht "umgeschrieben" wurde auf die Quelle

sidey79 commented 1 year ago

Ich bin ja schon dabei, das RDMsg Modul auf das neue Schema zu ändern :)

elektron-bbs commented 1 year ago

Die Tests liefen ja hier nun wieder nicht durch. Woran es liegt, kann ich leider nicht finden.

Auf jeden Fall sind in der testData.json keine Daten für DC-1961-TG mehr enthalten. Im SIGNALduino_TOOL waren sie am 12.12.22 noch enthalten https://github.com/RFD-FHEM/SIGNALduino_TOOL/commit/75d303b41454eeb205b7ef9803683f2aeffcecdb und wurden kurz darauf wieder entfernt https://github.com/RFD-FHEM/SIGNALduino_TOOL/commit/306d2562906bcedbca737f259db3d7f3559e2e99.

HomeAutoUser commented 1 year ago

Die Tests liefen ja hier nun wieder nicht durch. Woran es liegt, kann ich leider nicht finden.

Auf jeden Fall sind in der testData.json keine Daten für DC-1961-TG mehr enthalten. Im SIGNALduino_TOOL waren sie am 12.12.22 noch enthalten RFD-FHEM/SIGNALduino_TOOL@75d303b und wurden kurz darauf wieder entfernt RFD-FHEM/SIGNALduino_TOOL@306d256.

Da @sidey79 die Testdaten im testData.json haben möchte und im TOOL JSON diese nicht gewollt waren :-D grins so muss man nur die entfernten vom TOOL in die testData.json ergänzen.

sidey79 commented 1 year ago

Die Tests liefen ja hier nun wieder nicht durch. Woran es liegt, kann ich leider nicht finden.

Hab beim Mergen einen Konflikt übersehen. Der war schuld.

sidey79 commented 1 year ago

Die Tests liefen ja hier nun wieder nicht durch. Woran es liegt, kann ich leider nicht finden.

Habe beim Mergen einen Konflikt übersehen. Der war schuld. Ich habe die Testdaten hinzugefügt. Sollte dann jetzt alles fertig sein hier oder?

elektron-bbs commented 1 year ago

Warum das jetzt bei mir wieder bockt, verstehe ich nicht. Ich wollte eigentlich noch zweimal SD_BELL in den Metadaten ändern in SD_UT - übernimmst du das bitte? Ansonsten können wir das so übernehmen.

sidey79 commented 1 year ago

Das liegt daran, dass Du hier 35 commits eingefügt hast :( https://github.com/RFD-FHEM/RFFHEM/pull/1129/commits/2c10645eb553ff769d81f23d7f744b6912f00040

Den Branch auf den zuletzt richtigen Stand zurücksetzen würde das Problem beseitigen.

sidey79 commented 1 year ago

@elektron-bbs

Ich hab den Branch zurückgesetzt und die Testdaten wieder eingecheckt. Sollte jetzt wieder passen.

elektron-bbs commented 1 year ago

Alles klar, danke. Ich vergreife mich jetzt besser nicht nochmal daran, Github Desktop scheint da irgend etwas falsch zu machen. Änderst du bitte noch die beiden Einträge in den Metadaten vom 14_SD_UT.pm:

    "x_commandref": {
      "web": "https://commandref.fhem.de/#SD_BELL"

    "x_wiki": {
      "web": "https://wiki.fhem.de/wiki/SD_BELL"

SD_BELL -> SD_UT

Dann könnten wir mergen.

sidey79 commented 1 year ago

Alles klar, danke. Ich vergreife mich jetzt besser nicht nochmal daran, Github Desktop scheint da irgend etwas falsch zu machen.

Naja falsch würde ich so nicht sagen, aber wenn man einen merge in einem branch macht in dem ich an der Historie rumgefuhrwerkt hat, dann kommt vielleicht nicht das gewünschte dabei heraus ;)

Da ich GH Desktop selbst nicht nutze, kann ich nicht exakt sagen wie ich die Option nennt, aber vermutlich würde sowas wie reset current branch to xxx helfen. xxx wäre dann der Stand vom remote branch oder ein früherer commit.

Änderst du bitte noch die beiden Einträge in den Metadaten vom 14_SD_UT.pm:
    "x_commandref": {
      "web": "https://commandref.fhem.de/#SD_BELL"

    "x_wiki": {
      "web": "https://wiki.fhem.de/wiki/SD_BELL"

Den wiki Eintrag habe ich entfernt, da es diese Wikiseite nicht gibt, Commandref habe ich angepasst. Ich hoffe es stimmt nun alles.

elektron-bbs commented 1 year ago

Naja falsch würde ich so nicht sagen, aber wenn man einen merge in einem branch macht in dem ich an der Historie rumgefuhrwerkt hat, dann kommt vielleicht nicht das gewünschte dabei heraus ;)

So sieht es wohl aus...

Da ich GH Desktop selbst nicht nutze, kann ich nicht exakt sagen wie ich die Option nennt, aber vermutlich würde sowas wie reset current branch to xxx helfen. xxx wäre dann der Stand vom remote branch oder ein früherer commit.

Ich habe dann heute nochmal etwas gelesen - genau diesen reset scheint es bei Github-Desktop nicht zu geben.

Den wiki Eintrag habe ich entfernt, da es diese Wikiseite nicht gibt, Commandref habe ich angepasst. Ich hoffe es stimmt nun alles.

Ich denke ja, fehlt jetzt nur noch ein Review...

RFD-FHEM / RFFHEM

SD_UT remote control DC-1961-TG #1129

Codecov Report