elektron-bbs commented 2 years 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) Many sensors change their ident after a restart, e.g. when changing the battery. This creates a new sensor.
What is the new behavior (if this is a feature change)? The creation of new sensors after changing the battery can be prevented with this new command: set sensor replaceBatteryForSec seconds The device then waits the specified time in seconds until a sensor with the same protocol but unknown address is received. If this is successful, the definition of the device is changed.
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 2 years ago

Codecov Report

Merging #1074 (8687390) into master (34a2ada) will increase coverage by 0.23%. The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master    #1074      +/-   ##
==========================================
+ Coverage   63.46%   63.70%   +0.23%     
==========================================
  Files         128      130       +2     
  Lines        9436     9490      +54     
  Branches     1492     1500       +8     
==========================================
+ Hits         5989     6046      +57     
+ Misses       2301     2289      -12     
- Partials     1146     1155       +9

Flag	Coverage Δ
fhem	`55.46% <84.61%> (+0.36%)`	:arrow_up:
modules	`63.70% <84.61%> (+0.23%)`	:arrow_up:
perl	`90.61% <ø> (-0.09%)`	:arrow_down:
unittests	`63.70% <84.61%> (+0.23%)`	:arrow_up:

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

Impacted Files	Coverage Δ
FHEM/14_SD_WS.pm	`65.54% <83.33%> (+1.29%)`	:arrow_up:
t/FHEM/14_SD_WS/01_set.t	`100.00% <100.00%> (ø)`
FHEM/lib/SD_Protocols.pm	`79.47% <0.00%> (-0.23%)`	:arrow_down:
t/FHEM/14_FLAMINGO/09_parseDatat.t
t/FHEM/14_SD_BELL/09_parseDatat.t	`80.00% <0.00%> (ø)`
t/FHEM/14_FLAMINGO/00_load.t	`100.00% <0.00%> (ø)`
FHEM/10_SD_Rojaflex.pm	`72.76% <0.00%> (+4.06%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 34a2ada...8687390. Read the comment docs.

elektron-bbs commented 2 years ago

Wer ist bloß auf die Idee gekommen, Tabs durch Leerzeichen zu ersetzen :-) Jetzt sollten keine Tabs mehr zu finden sein.

sidey79 commented 2 years ago

Ich habe ein paar Tests für die neue sub erstellt, aber das Ergebnis passt nicht so recht.

elektron-bbs commented 2 years ago

Ich weiß nicht so richtig, was du testen willst. Auf jeden Fall reagiert das Modul erst, wenn LastInputDev gesetzt ist und das Attribut "longids" dazu passt. In allen anderen Fällen sollte ein undef zurück gegeben werden, b.z.w. das was carp daraus macht.

sidey79 commented 2 years ago

Ich weiß nicht so richtig, was du testen willst. Auf jeden Fall reagiert das Modul erst, wenn LastInputDev gesetzt ist und das Attribut "longids" dazu passt. In allen anderen Fällen sollte ein undef zurück gegeben werden, b.z.w. das was carp daraus macht.

Den Eindruck habe ich auch, allerdings muss get ? die möglichen Befehle liefern und undef ist dann nicht korrekt.

Eine Rückmeldung wäre ja vielleicht auch gut, wenn die longids nicht passend gesetzt ist oder war der Plan den Befehl erst anzubieten wenn alle Vorbedingungen erfüllt sind?

elektron-bbs commented 2 years ago

Letzteres ist richtig. Ich will den Befehl nur anbieten, wenn er Sinn macht. Bei Protokollen ohne longids braucht man ihn nicht und ohne LastInputDev funktioniert er nicht.

carp scheint hier übrigens nicht zu funktionieren:

  my @args = @_ // carp 'A minimum of one argument needs to be specified';

Ich erhalte dann immer 1 in $args[0] anstelle der eingegebenen Sekunden. Ich habe das jetzt wieder entfernt.

sidey79 commented 2 years ago

Letzteres ist richtig. Ich will den Befehl nur anbieten, wenn er Sinn macht. Bei Protokollen ohne longids braucht man ihn nicht und ohne LastInputDev funktioniert er nicht.

Ich habe die Tests darauf angepasst.

Ich erhalte dann immer 1 in $args[0] anstelle der eingegebenen Sekunden. Ich habe das jetzt wieder entfernt.

Das ist mir auch aufgefallen, wollte aber zunächst die Tests schreiben, bevor wir das an den subs ändern.

Die Tests der setFN stehen. Ich habe noch einen Test erstellt, welcher das ändern der DEF verifizieren soll. Der ist allerdings nicht erfolgreich, er überschreibt kein vorhandenes Device, sondern legt ein neues an. Entweder ich habe falsche Testdaten verwendet oder eine Fehler gefunden.

elektron-bbs commented 2 years ago

Ich erhalte dann immer 1 in $args[0] anstelle der eingegebenen Sekunden. Ich habe das jetzt wieder entfernt.

Das ist mir auch aufgefallen, wollte aber zunächst die Tests schreiben, bevor wir das an den subs ändern.

Ich habe im Netz dazu nichts gefunden. Da du in der SD_Protocols.pm das bei einem Array auch immer ohne carp machst, dachte ich, das das halt nicht funktioniert.

Die Tests der setFN stehen. Ich habe noch einen Test erstellt, welcher das ändern der DEF verifizieren soll. Der ist allerdings nicht erfolgreich, er überschreibt kein vorhandenes Device, sondern legt ein neues an. Entweder ich habe falsche Testdaten verwendet oder eine Fehler gefunden.

Keine Ahnung, mir fällt in dem Test auch erstmal nichts auf. Im "echten" FHEM habe ich es eben nochmal auf zwei Systemen getestet. Da funktioniert es.

sidey79 commented 2 years ago

Ich erhalte dann immer 1 in $args[0] anstelle der eingegebenen Sekunden. Ich habe das jetzt wieder entfernt.

Das ist mir auch aufgefallen, wollte aber zunächst die Tests schreiben, bevor wir das an den subs ändern.

Ich habe im Netz dazu nichts gefunden. Da du in der SD_Protocols.pm das bei einem Array auch immer ohne carp machst, dachte ich, das das halt nicht funktioniert.

Die Tests der setFN stehen. Ich habe noch einen Test erstellt, welcher das ändern der DEF verifizieren soll. Der ist allerdings nicht erfolgreich, er überschreibt kein vorhandenes Device, sondern legt ein neues an. Entweder ich habe falsche Testdaten verwendet oder eine Fehler gefunden.

Keine Ahnung, mir fällt in dem Test auch erstmal nichts auf. Im "echten" FHEM habe ich es eben nochmal auf zwei Systemen getestet. Da funktioniert es.

Mit den gleichen Daten?

elektron-bbs commented 2 years ago

Einmal mit einem SD_WS_33_T und zum zweiten mit den Testdaten des SD_WS_33_TH aus unserem SIGNALduino_Tool, welche du wahrscheinlich auch verwendest.

sidey79 commented 2 years ago

Einmal mit einem SD_WS_33_T und zum zweiten mit den Testdaten des SD_WS_33_TH aus unserem SIGNALduino_Tool, welche du wahrscheinlich auch verwendest.

Ich vermute, es liegt an dieser Prüfung:

        my $rproto = $rhash->{$ioname . '_Protocol_ID'};
        if (defined $rproto && $rproto eq $protocol) {

Ich finde keine Stelle, an der ein Internal mit dem Namen des empfangenden IO Devices + protocol_ID gesetzt wird.

elektron-bbs commented 2 years ago

Das könnte passen. Ich habe z.B. bei dem Sensor SD_WS_33_T_211_1 diese Internals:

sduinoACM_Protocol_ID 33 
sduinoIP_Protocol_ID 33 
sduinoUSB0_Protocol_ID 33

sidey79 commented 2 years ago

Das könnte passen. Ich habe z.B. bei dem Sensor SD_WS_33_T_211_1 diese Internals:
sduinoACM_Protocol_ID 33 
sduinoIP_Protocol_ID 33 
sduinoUSB0_Protocol_ID 33 

Hast Du auch gefunden, wo das gesetzt wird? Ich leider nicht. :(

elektron-bbs commented 2 years ago

Ich schätze mal, das wird in der 00_SIGNALduino.pm sub SIGNALduno_Dispatch in dieser Zeile

Dispatch($hash, $dmsg, \%addvals); ## Dispatch to other Modules

an die sub Dispatch in der fhem.pl übergeben und dann dort weiter verarbeitet:

            foreach my $av (keys %{$addvals}) {
              $defs{$found}{"${name}_$av"} = $addvals->{$av};
              push(@{$defs{$found}{CHANGED}}, "$av: $addvals->{$av}")
                if($avtrigger);
            }

elektron-bbs commented 2 years ago

Wenn wir richtig liegen, müsste es reichen, vor dem letzten "replaceBatteryForSec 10" eine Nachricht zu dispatchen. Dabei werden die Internals angelegt.

sidey79 commented 2 years ago

Wenn wir richtig liegen, müsste es reichen, vor dem letzten "replaceBatteryForSec 10" eine Nachricht zu dispatchen. Dabei werden die Internals angelegt.

Ja, das würde funktionieren, aber ich will nicht Dispatch testen :) Ich habe mich noch nicht entschieden, aber tendiere dazu das Internal im Test anzulegen.

sidey79 commented 2 years ago

Wenn wir richtig liegen, müsste es reichen, vor dem letzten "replaceBatteryForSec 10" eine Nachricht zu dispatchen. Dabei werden die Internals angelegt. @elektron-bbs

Ich habe das device nun so gemockt, dass der Test funktioniert.

Mir sind dabei aber noch zwei Dinge aufgefallen, die ich grundsätzlich diskutieren möchte.

1) Soll wir anstatt $ioname_Protocol_ID nicht eher prüfe, ob das IODev in LASTInputDev eingetragen ist? Letzteres ist ein Standard FHEM Internal.

2) Im Moment wird für jedes nicht bekannte Device eine Suche mittels devspec2array ausgelöst, auch wenn $longids nicht passt. Das deaktiviert autocreate für alle SD_WS Sensoren, solange einer mit replaceBattery belegt ist. Das kann ein Feature oder auch eine ungewollte Situation sein. Da wir dieses Verhalten nicht in der Commandref hinterlegt haben, würde ich dazu tendieren dass es keine Absicht war und devspec2array nur Ausführen wenn $longids einen passenden Wert besitzt.

elektron-bbs commented 2 years ago

Soll wir anstatt $ioname_Protocol_ID nicht eher prüfe, ob das IODev in LASTInputDev eingetragen ist? Letzteres ist ein Standard FHEM Internal.

Mhmm, ich wüsste jetzt nicht, inwiefern IODev ein Kriterium sein könnte, um zu prüfen, ob die Nachricht passt. Da ist ja gerade mal gewährleistet, das Frequenz und rfmode dazu passt. Ich wollte schon wenigstens prüfen, das es das richtige Protokoll ist. In dem Modul haben wir ja mittlerweile über 20 Protokolle.

Im Moment wird für jedes nicht bekannte Device eine Suche mittels devspec2array ausgelöst, auch wenn $longids nicht passt. Das deaktiviert autocreate für alle SD_WS Sensoren, solange einer mit replaceBattery belegt ist. Das kann ein Feature oder auch eine ungewollte Situation sein. Da wir dieses Verhalten nicht in der Commandref hinterlegt haben, würde ich dazu tendieren dass es keine Absicht war und devspec2array nur Ausführen wenn $longids einen passenden Wert besitzt.

Du meinst die Reihenfolge der Prüfungen so ändern?

  if(!$def) {
    if (($longids ne "0") && ($longids eq "1" || $longids eq "ALL" || (",$longids," =~ m/,$model,/))) {
      my @found = devspec2array("TYPE=SD_WS:FILTER=i:replaceBattery>0");
      if (scalar(@found) > 0) {

Das funktioniert genau so. Kann ich ändern.

sidey79 commented 2 years ago

Das funktioniert genau so. Kann ich ändern.

Ja, das wäre gut

elektron-bbs commented 2 years ago

Erledigt, deine Änderungen scheinen auch zu funktionieren :-)

RFD-FHEM / RFFHEM

14_SD_WS.pm new set command replaceBatteryForSec #1074

Codecov Report