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) https://github.com/RFD-FHEM/SIGNALduino_TOOL/commit/b00fad145477736e625029e523afe7223b19877c
[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
[x] Unittest enhancement
[ ] other
What is the current behavior? (You can also link to an open issue here, if this describes the current behavior) Incomplete decoding of messages from an unknown remote control, see https://forum.fhem.de/index.php/topic,53282.msg1233431.html#msg1233431
What is the new behavior (if this is a feature change)? Receiving and sending the messages.
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 #1119 (bd8b3d8) into master (115b152) will decrease coverage by 0.12%. The diff coverage is 64.22%.

@@            Coverage Diff             @@
##           master    #1119      +/-   ##
==========================================
- Coverage   65.21%   65.09%   -0.13%     
==========================================
  Files         139      140       +1     
  Lines        9783     9886     +103     
  Branches     1550     1562      +12     
==========================================
+ Hits         6380     6435      +55     
- Misses       2182     2206      +24     
- Partials     1221     1245      +24

Flag	Coverage Δ
fhem	`57.28% <64.22%> (-0.05%)`	:arrow_down:
modules	`65.09% <64.22%> (-0.13%)`	:arrow_down:
perl	`90.33% <ø> (ø)`
unittests	`65.09% <64.22%> (-0.13%)`	:arrow_down:

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% <ø> (ø)`
t/FHEM/14_SD_UT/01_attr.t	`100.00% <ø> (ø)`
lib/Test2/SIGNALduino/FHEM_Command.pm	`51.47% <51.47%> (ø)`
FHEM/14_SD_UT.pm	`44.82% <82.14%> (+4.27%)`	:arrow_up:
t/FHEM/14_SD_UT/03_set.t	`92.30% <92.30%> (ø)`
FHEM/14_SD_WS09.pm	`64.18% <0.00%> (-8.90%)`	:arrow_down:
FHEM/10_SD_Rojaflex.pm	`71.95% <0.00%> (-0.41%)`	:arrow_down:
t/FHEM/14_BresserTemeo/09_parseDatat.t
t/FHEM/98_Dooya/00_load.t
t/FHEM/14_BresserTemeo/00_load.t
... and 3 more

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

elektron-bbs commented 1 year ago

@sidey79 Das hatten wir ja gerade erst: codecov/patch — 30.00% of diff hit (target 55.62%) Würde es helfen, die vielen Kommentarzeilen zu entfernen? Ich weiß nicht, wie das Teil bewertet!

sidey79 commented 1 year ago

Seltsam dass es gefühlt immer 30% sind.

Ich verstehe das so, von der Änderung sind 30% nicht durch die Tests ausgeführt worden. Der Schwellwert liegt bei 10%. Kommentare zählen hier nicht mit rein.

Es handelt sich insbesondere um den Abschnitt mit RCnoName20_10

elektron-bbs commented 1 year ago

Wenn ich das richtig sehe (die rot markierten Stellen - Zeile 1221 bis 1410) befinden sich ja alle in der sub SD_UT_Set. Da müsste man ja für jede Fernbedienung einen Sendebefehl testen und zum Schluss z.B. $msg prüfen - hast du so etwas schon in den Tests?

sidey79 commented 1 year ago

Wenn ich das richtig sehe (die rot markierten Stellen - Zeile 1221 bis 1410) befinden sich ja alle in der sub SD_UT_Set. Da müsste man ja für jede Fernbedienung einen Sendebefehl testen und zum Schluss z.B. $msg prüfen - hast du so etwas schon in den Tests?

So genau hatte ich nicht nachgesehen, aber ja es ist innerhalb der SET Funktion.

Für SD_UT gibt es noch nichts, wir haben etwas für das SD_WS Modul: https://github.com/RFD-FHEM/RFFHEM/blob/master_RCnoName20_10/t/FHEM/14_SD_WS/01_set.t

Etwas generisches wäre natürlich schick, denn jedesmal das Rad neu zu definieren macht keinen Spaß: https://github.com/RFD-FHEM/RFFHEM/blob/master_RCnoName20_10/t/FHEM/00_SIGNALduino/03_SIGNALduino_Set.t

Das ist ja schon etwas generischer. Beim SET Befehl ist es ja aber so eine Sache, wie das korrekte Ergebnis geprüft wird. Der Return Wert selbst ist ja nicht wirklich aussagefähig glaube ich, das Ergebnis wird ja an IOWrite übergeben und das was wir hier hin liefern ist ja ausschlaggebend, ob es stimmt.

elektron-bbs commented 1 year ago

Für SD_UT gibt es noch nichts, wir haben etwas für das SD_WS Modul: https://github.com/RFD-FHEM/RFFHEM/blob/master_RCnoName20_10/t/FHEM/14_SD_WS/01_set.t

Das ist ziemlich speziell, mit nur einem Set-Befehl....

Etwas generisches wäre natürlich schick, denn jedesmal das Rad neu zu definieren macht keinen Spaß: https://github.com/RFD-FHEM/RFFHEM/blob/master_RCnoName20_10/t/FHEM/00_SIGNALduino/03_SIGNALduino_Set.t Das ist ja schon etwas generischer. Beim SET Befehl ist es ja aber so eine Sache, wie das korrekte Ergebnis geprüft wird. Der Return Wert selbst ist ja nicht wirklich aussagefähig glaube ich, das Ergebnis wird ja an IOWrite übergeben und das was wir hier hin liefern ist ja ausschlaggebend, ob es stimmt.

Das sehe ich auch so. Im Prinzip müsste die komplette sub SD_UT_Set in den Test übernommen werden, da jedes Protokoll unterschiedlich aufgebaut ist....

elektron-bbs commented 1 year ago

@sidey79 Hast du mittlerweile eine Idee, wie das Problem zu lösen ist?

sidey79 commented 1 year ago

@elektron-bbs

Idee hätte ich da grob ja. Irgendwie hatte ich angenommen dass Du etwas erstellst. Keine Ahnung wieso ich das dachte, daher habe ich jedenfalls nichts gemacht :)

Ich nehme also an, ich soll mal was generisches entwerfen.

elektron-bbs commented 1 year ago

Das wäre nicht schlecht. Ich habe keine Ahnung, wie das mit den Tests funktioniert.

sidey79 commented 1 year ago

Das wäre nicht schlecht. Ich habe keine Ahnung, wie das mit den Tests funktioniert.

Ich musste mich da auch erst einmal reindenken. Beim SET Befehl ist es auch nicht unbedingt so einfach, da es keinen Rückgabewert gibt. :( Bin aber dran.

elektron-bbs commented 1 year ago

Naja, nur um den Test zu "befriedigen" könnte man ja auch erstmal nur die Readings verwenden:

Internals:
   DEF        RCnoName20_10 3E00
   FUUID      63209e84-f33f-b27f-7a13-bb9bb28d590a64b8
   IODev      sduinoUSB0
   LASTInputDev sduinoACM
   MSGCNT     1
   NAME       RCnoName20_10_3E00
   NR         469
   STATE      fan_high
   TYPE       SD_UT
   bitMSG     00111110000000000001010000110001
   eventCount 2
   lastMSG    3E001431
   sduinoACM_DMSG P20#3E001431
   sduinoACM_MSGCNT 1
   sduinoACM_Protocol_ID 20
   sduinoACM_RAWMSG MS;P0=-243;P2=-7929;P5=-729;P6=229;P7=715;D=626565707070707065656565656565656565656570657065656565707065656570;CP=6;SP=2;R=237;O;m1;
   sduinoACM_RSSI -83.5
   sduinoACM_TIME 2022-10-16 21:01:43
   sduinoESP8266_DMSG P20#3E001431
   sduinoESP8266_MSGCNT 1
   sduinoESP8266_Protocol_ID 20
   sduinoESP8266_RAWMSG MS;P0=705;P1=-238;P2=230;P3=-728;P6=-7879;D=262323010101010123232323232323232323232301230123232323010123232301;CP=2;SP=6;R=42;m2;
   sduinoESP8266_RSSI -53
   sduinoESP8266_TIME 2022-10-16 21:01:43
   versionModule 2022-09-13
   Helper:
     DBLOG:
       RSSI:
         myDbLog:
           TIME       1665946903.52261
           VALUE      -83.5
   READINGS:
     2022-10-05 21:14:18   IODev           sduinoUSB0
     2022-10-16 21:01:43   LastAction      receive
     2022-10-16 21:01:43   deviceCode      3E00
     2022-10-16 21:01:43   rollingCode     3
     2022-10-16 21:01:43   state           fan_high
Attributes:
   IODev      sduinoUSB0
   model      RCnoName20_10
   room       Test

Das Reading "LastAction" wäre dann in Fall des Sendens halt auf "send" und müsste eigentlich bei allen Protokollen des Modules passen. Das Reading "state" sollte auch bei allen Protokollen den gleichnamigen Set-Befehl wiedergeben. "deviceCode" findet sich im Internal "DEF". "rollingCode" gibt es bisher nur bei diesem Protokoll.

sidey79 commented 1 year ago

Ich kann das Ergebnis bereits verifizieren indem ich prüfe was an IOWrite gesendet wird. Allerdings ist mir das im Moment noch zu viel Code der in jedem Test definiert werden muss, das möchte ich schlanker gestalten.

sidey79 commented 1 year ago

Das Testtool wäre nun verfügbar. Es ist halt leider try & Error um die richtigen sendMsg Parameter zu erwischen. Zumindest für mich :)

sidey79 commented 1 year ago

@HomeAutoUser

Also die Tests müssten noch erstellt werden meine ich.

HomeAutoUser commented 1 year ago

@sidey79 Wenn die Tests noch gemacht werden müssen, dann sieh den Approve als „hier ich lese mit über die Änderungen“ ;-) Dann weiter an das umsetzen der Tests. Du hast ja schon den Anfang geschaffen.

sidey79 commented 1 year ago

@sidey79

Alles supi. Ich habe eine vermeintlich einfache Möglichkeit geschaffen die Befehle zu testen. Ich blicke aber nicht durch, welche Bedingungen es im Fall dieser Fernbedienung hier gibt.

Meine Hoffnung ist, @elektron-bbs kann die nötigen Tests definieren und den PR zum Abschluss bringen.

elektron-bbs commented 1 year ago

Mhmm, was braucht es denn noch für Tests? codecov ist doch jetzt zufriedengestellt:

vorher: codecov/patch — 30.00% of diff hit (target 55.62%)
jetzt:  codecov/patch — 73.33% of diff hit (target 55.62%)

sidey79 commented 1 year ago

Mhmm, was braucht es denn noch für Tests? codecov ist doch jetzt zufriedengestellt:
vorher: codecov/patch — 30.00% of diff hit (target 55.62%)
jetzt:  codecov/patch — 73.33% of diff hit (target 55.62%)

Nicht so sehr auf die % achten. Die gehen nur noch, weil das Testtool direkt hierer gebracht habe, es aber im master nicht läuft.

Letztendlich braucht es für die in diesem PR hinzugefügt Funktionalitäten einen Test.

elektron-bbs commented 1 year ago

Dann schreib mir bitte, was ich tun soll.

sidey79 commented 1 year ago

@elektron-bbs

Aus meiner Sicht sind wir hier mit Tests jetzt fertig.

RFD-FHEM / RFFHEM

SD_UT, new remote control RCnoName20_10 #1119

Codecov Report