14_SD_WS.pm new set command replaceBatteryForSec #1074

elektron-bbs · 2022-02-05T16:05:35Z

Please check if the PR fulfills these requirements

Tests for the changes have been added / modified (needed for for bug fixes / features)
commandref has been added / updated (needed for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

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 · 2022-02-05T16:07:13Z

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%)`	⬆️
modules	`63.70% <84.61%> (+0.23%)`	⬆️
perl	`90.61% <ø> (-0.09%)`	⬇️
unittests	`63.70% <84.61%> (+0.23%)`	⬆️

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%)`	⬆️
t/FHEM/14_SD_WS/01_set.t	`100.00% <100.00%> (ø)`
FHEM/lib/SD_Protocols.pm	`79.47% <0.00%> (-0.23%)`	⬇️
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%)`	⬆️

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.

FHEM/14_SD_WS.pm

sidey79

Da gibt es noch ein paar Zeilen, die scheinen falsch eingerückt,
Könnte so ein tab vs leerzeichen Thema sein

elektron-bbs · 2022-02-06T15:18:02Z

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

…FHEM/RFFHEM into master_SD_WS_replaceBattery

added some basic test for set command

sidey79 · 2022-02-09T22:42:36Z

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

elektron-bbs · 2022-02-10T16:32:54Z

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.

Tests angepasst, so dass diese das dokumentierten testen

changed quotes

added device with longids for test

testdevice mit longid hinterlegt

sidey79 · 2022-02-11T22:32:28Z

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 · 2022-02-12T12:30:40Z

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 · 2022-02-12T17:41:22Z

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 · 2022-02-12T19:31:22Z

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 · 2022-02-13T16:07:30Z

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 · 2022-02-13T17:26:56Z

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 · 2022-02-13T18:29:25Z

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 · 2022-02-13T20:03:33Z

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 · 2022-02-14T15:56:40Z

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

sidey79 · 2022-02-14T17:28:45Z

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.

mocked device under test for replaceBattery check

little improvement for replaceBattery check

…FHEM/RFFHEM into master_SD_WS_replaceBattery

sidey79 · 2022-02-15T20:46:32Z

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.

Soll wir anstatt $ioname_Protocol_ID nicht eher prüfe, ob das IODev in LASTInputDev eingetragen ist?
Letzteres ist ein Standard FHEM Internal.
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 · 2022-02-16T16:27:02Z

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 · 2022-02-16T18:36:49Z

Das funktioniert genau so. Kann ich ändern.

Ja, das wäre gut

elektron-bbs · 2022-02-16T19:40:09Z

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

elektron-bbs and others added 2 commits February 5, 2022 16:52

14_SD_WS.pm new set command replaceBatteryForSec

f8f41cd

Automatic updated controls and CHANGED

821af8b

elektron-bbs requested a review from sidey79 February 5, 2022 16:14