Skip to content
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

14_SD_WS.pm: New protocol for Atech wireless weather station #578

Merged
merged 16 commits into from
May 14, 2019
Merged

14_SD_WS.pm: New protocol for Atech wireless weather station #578

merged 16 commits into from
May 14, 2019

Conversation

elektron-bbs
Copy link
Contributor

@elektron-bbs elektron-bbs commented May 2, 2019

  • 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)
  • CHANGED has been updated (needed for bug fixes / features)

feature: 14_SD_WS.pm: New protocol for Atech wireless weather station (protocol #94)

  • 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)
    Atech wireless weather station (vermutlicher Name: WS-308) #547

  • What is the new behavior (if this is a feature change)?

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    no

  • Other information:

elektron-bbs and others added 7 commits March 6, 2019 20:51
14_SD_UT.pm: new protocol for Techmar Garden Lights
SD_ProtocolData.pm: set protocol 94 to development
SD_ProtocolData.pm: new definition protocol 95
README.md: new entries for Tedsen, Geiger and Techmar
…#94)

14_SD_WS.pm: New protocol for Atech wireless weather station (protocol
#94)
@elektron-bbs elektron-bbs requested a review from sidey79 May 2, 2019 19:01
@sidey79
Copy link
Contributor

sidey79 commented May 2, 2019

Der Travis Build Fehler ist nicht neu, der ist leider schon alt nur aufgrund eines Fehlers nicht aufgetreten.
Ich muss mir noch ansehen, von wo genau er kommt.

@elektron-bbs
Copy link
Contributor Author

Ich hab eben schon nach meiner Version gesehen:
perl 5, version 24, subversion 1 (v5.24.1) built for arm-linux-gnueabihf -thread-multi-64int
Da funktioniert es. Aus den Meldungen von Travis werde ich nicht schlau.

@sidey79
Copy link
Contributor

sidey79 commented May 2, 2019

Du meinst diese Meldung:

Can't use a hash as a reference at (eval 123)[./FHEM/98_unittest.pm:168] line 1.

Die besagt, dass wir einen hash nicht dereferenzieren können. Seit Perl 5.24 ist das nicht mehr zulässig.
Vorher kamen nur "Warnings" Was genau geht da denn bei dir mit 5.24 ?

@elektron-bbs
Copy link
Contributor Author

Ich meinte damit, das FHEM mit dieser Perl-Version bei mir nichts zu mäkeln hat. Die Tests von dir lasse ich hier nicht laufen.

@sidey79
Copy link
Contributor

sidey79 commented May 2, 2019

Ok verstehe. Ich teste lokal auf 5.18. Müsste mir mal überlegen wie ich lokal auch mit anderen Versionen testen könnte. Zum debuggen wäre das einfacher.

Ich bin dabei das zu beheben, allerdings komme ich von einem Fehler zum Nächsten :(

@sidey79
Copy link
Contributor

sidey79 commented May 3, 2019

@elektron-bbs
ich habe #579 fertig. Damit werden die Tests selbst korrigiert.

Der Ablauf wäre so:
579 -> dev-r34 -> elektron-bbs:dev-r34

@elektron-bbs
Copy link
Contributor Author

Muss ich jetzt erneut einen PR tätigen?

@sidey79
Copy link
Contributor

sidey79 commented May 4, 2019

nein, Wir müssen nur deinen Branch mit den Aktualisierungen aus dev-r34 aktualisieren. Soll ich das machen oder weisst Du wie es geht?

@elektron-bbs
Copy link
Contributor Author

Wie das geht, habe ich bis heute nicht heraus bekommen. Ich habe jetzt meistens bei mir das Repo gelöscht und dann neu geforkt um auf den aktuellen Stand zu kommen.

@coveralls
Copy link

coveralls commented May 4, 2019

Pull Request Test Coverage Report for Build 1470

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at ?%

Totals Coverage Status
Change from base Build 1462: 0.0%
Covered Lines:
Relevant Lines: 0

💛 - Coveralls

FHEM/14_SD_WS.pm Show resolved Hide resolved
FHEM/14_SD_WS.pm Show resolved Hide resolved
FHEM/14_SD_WS.pm Outdated Show resolved Hide resolved
FHEM/14_SD_WS.pm Outdated Show resolved Hide resolved
@sidey79
Copy link
Contributor

sidey79 commented May 4, 2019

Wie das geht, habe ich bis heute nicht heraus bekommen. Ich habe jetzt meistens bei mir das Repo gelöscht und dann neu geforkt um auf den aktuellen Stand zu kommen.

naja, Du müsstest deinen Zweig also elektron-bbs:dev-r34 auschecken und dann sowas wie

git remote add rfdfhem https://github.com/RFD-FHEM/RFFHEM.git
git merge rfdfhem/dev-r34

Dann legst Du eine neue Vebindung zu dem "haupt" Repo an und implementierst anschließend die Änderungen von dem Hauptrepo in deines.
Vermutlich hast Du auch bereits eine Verbindung zu dem "haupt" Repo mit einer bezeichnung.

elektron-bbs and others added 2 commits May 5, 2019 13:22
without longid: SD_WS_94_T
with longids: SD_WS_94_T_0C
Decoding code moved to decodingSubs
@sidey79
Copy link
Contributor

sidey79 commented May 10, 2019

@elektron-bbs

Ich hab den Code umgeschrieben.

Ich hab vor meine Anpassung und nach meiner Anpassung ein dispatch gemacht. Vorher kam da aber keine Feuchtigkeit. Jetzt kommt eine. Der Sensor sollte das ja auch haben oder?

2019-05-10 21:19:01 SIGNALduino_TOOL sdtool last_DMSG: W94#0D8031B60C6C
2019-05-10 21:19:01 SIGNALduino_TOOL sdtool message_dispatched: 1
2019-05-10 21:19:03 SD_WS SD_WS_94_T T: 27 H: 38
2019-05-10 21:19:03 SD_WS SD_WS_94_T temperature: 27
2019-05-10 21:19:03 SD_WS SD_WS_94_T humidity: 38
2019-05-10 21:19:03 SD_WS SD_WS_94_T batteryState: ok

FHEM/14_SD_WS.pm Outdated
($_[1] = $_[1]) =~ s/110/1/g;
return sprintf('%02X', SD_WS_bin2dec(substr($_[1],0,8)));
},
bat => sub { return substr($_[1],8,1) eq "0" ? "ok" : "low"; },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wie kommst du darauf, das es ein Batteriebit gäbe? Hast du eine Doku zum Protokoll gefunden? Im Issue Atech wireless weather station findet sich auch kein Hinweis dazu.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hab da wohl zu viel kopiert. Ich werde es wieder entfernen

FHEM/14_SD_WS.pm Outdated
};
my $temp = ($rawtemp100 * 10 + $rawtemp10 + $rawtemp1 / 10) * ( substr($_[1],10,1) == 1 ? -1.0 : 1.0);
},
hum => sub {my (undef,$bitData) = @_; return SD_WS_binaryToNumber($bitData,24,31); },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich denke nicht, das das ein Feuchtewert sein könnte. Im Issue ist auch nie die Rede davon, das der Sensor eine Feuchte überträgt. Außerdem werden nicht immer 32 Bit übertragen (siehe Excel-Tabelle im Beitrag ).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hab da wohl zu viel kopiert. Ich werde es wieder entfernen

remove wrong added bat and humidity value for model SD_WS_94_T
@sidey79
Copy link
Contributor

sidey79 commented May 12, 2019

@elektron-bbs

Ich habe battery und humidity wieder entfernt

@elektron-bbs
Copy link
Contributor Author

Welche Änderungen möchtest du jetzt noch?
Den DeviceCode hatte ich geändert:
de4b8f0

@sidey79
Copy link
Contributor

sidey79 commented May 13, 2019

@elektron-bbs

Wenn zwei editieren, dann ist das immer so eine Sache. Wenn von deiner Seite her meine Anpassungen für dich okay sind dann können wir da einen Haken dran machen.

Von meiner Seite ist die Änderungen auch soweit okay. Ich halte eine Ergänzung in der commandref noch für wertvoll welches neue Gerät nun unterstützt wird. Machst Du die noch?

@elektron-bbs
Copy link
Contributor Author

Kann ich machen. Es gibt nur leider keinerlei sinnvolle Bezeichnung für diese Station/Sensor. Außer dem Hersteller "Atech" ist nichts bekannt. Schreib ich halt "Atech wireless weather station" hin.

Der Code funktioniert. Allerdings halte ich es weiterhin für weniger effizient, 6 subs aufzurufen statt eines "if". Kannst du mir den Vorteil erläutern?

@sidey79
Copy link
Contributor

sidey79 commented May 13, 2019

Der Vorteil liegt darin, dass alle Modelle nach dem gleichen Schema verarbeitet werden:

RFFHEM/FHEM/14_SD_WS.pm

Lines 773 to 803 in 27e1ec8

elsif (defined($decodingSubs{$protocol})) # durch den hash decodieren
{
$SensorTyp=$decodingSubs{$protocol}{sensortype};
if (!$decodingSubs{$protocol}{prematch}->( $rawData ))
{
Log3 $iohash, 4, "$name: SD_WS_Parse $rawData protocolid $protocol ($SensorTyp) - ERROR prematch" ;
return "";
}
my $retcrc=$decodingSubs{$protocol}{crcok}->( $rawData,$bitData );
if (!$retcrc) {
Log3 $iohash, 4, "$name: SD_WS_Parse $rawData protocolid $protocol ($SensorTyp) - ERROR CRC";
return "";
}
$id=$decodingSubs{$protocol}{id}->( $rawData,$bitData );
#my $temphex=$decodingSubs{$protocol}{temphex}->( $rawData,$bitData );
$temp=$decodingSubs{$protocol}{temp}->( $rawData,$bitData ) if (exists($decodingSubs{$protocol}{temp}));
$hum=$decodingSubs{$protocol}{hum}->( $rawData,$bitData ) if (exists($decodingSubs{$protocol}{hum}));
$windspeed=$decodingSubs{$protocol}{windspeed}->( $rawData,$bitData ) if (exists($decodingSubs{$protocol}{windspeed}));
$channel=$decodingSubs{$protocol}{channel}->( $rawData,$bitData ) if (exists($decodingSubs{$protocol}{channel}));
$model = $decodingSubs{$protocol}{model};
$bat = $decodingSubs{$protocol}{bat}->( $rawData,$bitData ) if (exists($decodingSubs{$protocol}{bat}));
$beep = $decodingSubs{$protocol}{beep}->( $rawData,$bitData ) if (exists($decodingSubs{$protocol}{beep}));
if ($model eq "SD_WS_33_T") { # for SD_WS_33 discrimination T - TH
$model = $decodingSubs{$protocol}{model}."H" if $hum != 0; # for models with Humidity
}
$sendmode = $decodingSubs{$protocol}{sendmode}->( $rawData,$bitData ) if (exists($decodingSubs{$protocol}{sendmode}));
$trend = $decodingSubs{$protocol}{trend}->( $rawData,$bitData ) if (exists($decodingSubs{$protocol}{trend}));
Log3 $iohash, 4, "$name: SD_WS_Parse decoded protocol-id $protocol ($SensorTyp), sensor-id $id";
}

Es ist nicht notwendig, sich mit der Zuweisung von den Variablen oder der Einhaltung einer Reihenfolge beschäftigen.

Stattdessen kann man auf diese Blaupause zurückgreifen und kümmert sich "nur" noch darum, dass die passenden Werte aus der Nachricht extrahiert werden.

RFFHEM/FHEM/14_SD_WS.pm

Lines 458 to 475 in 27e1ec8

sensortype => 'Atech',
model => 'SD_WS_94_T',
prematch => sub { return 1; }, # no precheck known
id => sub { # change 110 to 1 in ref bitdata and return id
($_[1] = $_[1]) =~ s/110/1/g;
return sprintf('%02X', SD_WS_bin2dec(substr($_[1],0,8)));
},
temp => sub {
my $rawtemp100 = SD_WS_binaryToNumber($_[1],12,15);
my $rawtemp10 = SD_WS_binaryToNumber($_[1],16,19);
my $rawtemp1 = SD_WS_binaryToNumber($_[1],20,23);
if ($rawtemp100 > 9 || $rawtemp10 > 9 || $rawtemp1 > 9) {
Log3 $iohash, 3, "$name: SD_WS_Parse $model ERROR - BCD of temperature ($rawtemp100 $rawtemp10 $rawtemp1)";
return "";
};
my $temp = ($rawtemp100 * 10 + $rawtemp10 + $rawtemp1 / 10) * ( substr($_[1],10,1) == 1 ? -1.0 : 1.0);
},
crcok => sub {return 1;}, # crc test method is so far unknown

Ich finde diese elendig langen If / then / Else Blöcke sehr schlecht lesbar und auch schlecht wartbar.

sidey79
sidey79 previously approved these changes May 13, 2019
@sidey79
Copy link
Contributor

sidey79 commented May 13, 2019

@elektron-bbs
Die ganzen zwischen commit messages kannst Du ersatzslos streichen. Es zählt nur das Endergebnis :)

@elektron-bbs
Copy link
Contributor Author

@sidey79
Du musst leider nochmal bestätigen, da ich die CHANGED ändern musste :-(

@elektron-bbs elektron-bbs merged commit 79c999c into RFD-FHEM:dev-r34 May 14, 2019
@sidey79
Copy link
Contributor

sidey79 commented May 14, 2019

@elektron-bbs

Wenn ich eine Anregung anbringen dürfte:

Der squash Merge commit hat den Zweck, dass man ein commit erzeugt und auch die Beschreibung anpasst.

Du hast jetzt diesen commit 79c999c mit folgendem Text hinzugefügt:

* new protocol for Techmar Garden Lights

14_SD_UT.pm: new protocol for Techmar Garden Lights
SD_ProtocolData.pm: set protocol 94 to development
SD_ProtocolData.pm: new definition protocol 95
README.md: new entries for Tedsen, Geiger and Techmar

* 14_SD_WS.pm: New protocol for Atech wireless weather station (protocol #94)

14_SD_WS.pm: New protocol for Atech wireless weather station (protocol
#94)

* change deviceCode

without longid: SD_WS_94_T
with longids: SD_WS_94_T_0C

* 14_SD_WS.pm

Decoding code moved to decodingSubs

* 14_SD_WS.pm

remove wrong added bat and humidity value for model SD_WS_94_T

* add model "Atech" to commandref

Der commit selbst beinhaltet ja nur die absoluten Änderungen zwischen dem feature branch und dem dev-r34 branch.

Von protocol 95 ist in dem commit ja jetzt nichts mehr enthalten. Und Anpassungen der Readme habe ich keine gefunden :(

Ausreichend und korrekter wäre etwas wie folgt gewesen. Die jetzt vorhandene commit message stimmt ja nicht und beinhaltet Dinge, die nicht im commit nicht enthalten sind.

*SD_ProtocolData.pm: 
new  protocol 94 

*14_SD_WS.pm: 
New device for Atech wireless weather station (protocol 94)

Wäre es möglich, die Nachricht in Zukunft auf die Endgültige Änderung zu optimieren?

@elektron-bbs
Copy link
Contributor Author

Sorry, mir war nicht bewusst, das ich die Beschreibung überhaupt editieren kann. Ich werde beim nächsten Mal darauf achten.
Wie kann ich verhindern, das Commits anderer User überhaupt solch ein Durcheinander anrichten? Würde es reichen, für jeden Pull request einen neuen Branch anzulegen?

@sidey79
Copy link
Contributor

sidey79 commented May 15, 2019

Ja, du kannst einen neuen Branch anlegen.
Solltest Du auch.

Ich verwende tortoise git. Beim Commit kann man den neuen Branch angeben
Es geht natürlich auch mit der Kommandozeile.

@elektron-bbs
Copy link
Contributor Author

OK.
Bei tortoise git sehe ich noch nicht so richtig durch. Ich verwende Github Desktop. Leider wird die 32Bit-Version nicht mehr weiter entwickelt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants