verify 1st Revolt checksum byte

[gernot-h]

NOTE: This is a port of a patch developed for the svn.fhem.de version of SIGNALduino code, see also https://forum.fhem.de/index.php/topic,58397.msg1151053.html#msg1151053. I'm interested in getting this change into the SIGNALduino code distributed via svn.fhem.de, so I hope this project is the place to prepare changes for FHEM integration!

Change description:

To avoid wrong reads and especially invalid autocreates for Revolt NC-546x
energy cost meters, add verification of the 1st Revolt checksum byte.
Check based on (old) analysis from mehf, see Forum #14292. There's
another checksum(?) byte which I didn't understand yet.

Also increase minimum length of messages to assure we received the checksum
byte. In general, this patch might slightly decrease probability of receiving
far devices, but I think avoiding erronous reads is more important.

• 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)

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

Fix the frequent reception of invalid messages leading to autocreation of non-existing devices by verifying the checksum.

• What is the current behavior? (You can also link to an open issue here)

I see frequent additions of non-existant Revolt devices (perhaps one per week) obviously caused by reception errors. Here's an example from a FHEM instance actually receiving two devices:

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

Drop messages without checksum byte or with invalid checksum.

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

There might be seldom cases where this change prevents reception of far devices with incomplete messages as we'll drop any message without checksum byte which could be parsed before introducing my change.

[codecov]

Codecov Report

Merging #956 (0743d30) into master (a725963) will increase coverage by 0.17%.
The diff coverage is 97.29%.

@@            Coverage Diff             @@
##           master     #956      +/-   ##
==========================================
+ Coverage   58.15%   58.32%   +0.17%     
==========================================
  Files         105      106       +1     
  Lines        8409     8446      +37     
  Branches     1314     1315       +1     
==========================================
+ Hits         4890     4926      +36     
  Misses       2634     2634              
- Partials      885      886       +1     

Flag	Coverage Δ
fhem	48.50% <75.00%> (+0.05%)	⬆️
modules	58.32% <97.29%> (+0.17%)	⬆️
perl	91.52% <97.29%> (+0.10%)	⬆️
unittests	58.32% <97.29%> (+0.17%)	⬆️

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% <ø> (ø)
FHEM/lib/SD_Protocols.pm	79.01% <93.75%> (+0.29%)	⬆️
t/SD_Protocols/02_postDemo_Revolt.t	100.00% <100.00%> (ø)

---

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 a725963...0743d30. Read the comment docs.

[gernot-h]

Shall I also report an issue for this?

TIA for your feedback!

[sidey79]

@gernot-h

Hi Gerno, tanks for your PR.

Issue isn't needed, but we need some of the data which has the wrong checksum and maybe also some data which is decoded.

Currently we have this data for tests:
https://github.com/RFD-FHEM/SIGNALduino_TOOL/blob/33ab9464e3ba54c6d2572764c13c3553d240b7b9/FHEM/lib/SD_Device_ProtocolList.json#L698

I think we will write a test against the new sub. Noting really complex, but similar to them to cover nearby all code paths in the new sub.

https://github.com/RFD-FHEM/RFFHEM/blob/master/t/SD_Protocols/02_postDemo_EM.t

What i see so far, the tests, which are currently running, had no problem with your change.

[gernot-h]

Perfect, thanks for the super-quick feedback!!

For now, here are two examples of valid messages from two different devices:

2021.04.11 14:58:53 4: sduino2: Read, msg READredu: MU;P0=-832;P1=10064;P2=-380;P3=108;P4=-257;P5=233;D=012343454345434545454543434543454345454543434543454343434343434343434343434343434343434545434345434343434343434343434343434343434343434343434343434543454543434543454343434543434543454343434545454343434343454;CP=3;R=0;
2021.04.11 14:58:53 4: sduino2: Parse_MU, Fingerprint for MU protocol id 45 -> Revolt matches, trying to demodulate
2021.04.11 14:58:53 4: sduino2: Parse_MU, Decoded matched MU protocol id 45 dmsg r2BCAE5000032000000B289 length 88 dispatch(1/4) RSSI = -74

2021.04.21 03:55:43 4: sduino2: Read, msg READredu: MU;P0=143;P1=-218;P2=11620;P3=-332;P5=264;D=01230151515101015151015101515101510151515101015101010101010101010101010101010101010101015151010151010101010101010101010101010101010101010101010101010151015101010151010101015151015101510101010101510151015151010150;CP=0;R=0;
2021.04.21 03:55:43 4: sduino2: Parse_MU, Fingerprint for MU protocol id 45 -> Revolt matches, trying to demodulate
2021.04.21 03:55:43 4: sduino2: Revolt, 103 bits received
2021.04.21 03:55:43 4: sduino2: Parse_MU, Decoded matched MU protocol id 45 dmsg r735AE4000032000000510D length 88 dispatch(1/4) RSSI = -74

Plus two examples of invalid messages from the first device, so they should start with r2BCA, not r2000 or r4000:

(The first lacks the CRC, so would be blocked anyways after changing min_length.)

2021.04.11 16:04:27 4: sduino2: Read, msg READredu: MU;P0=-272;P1=221;P3=-573;P5=10132;P6=-364;P7=94;D=356707010707070707070707070707070707070707070107010707070707070707070707070707070707070101070701070707070707070707070707070707070707070707070707070107010107070107010707070107070100;CP=7;R=0;
2021.04.11 16:04:27 4: sduino2: Parse_MU, Fingerprint for MU protocol id 45 -> Revolt matches, trying to demodulate
2021.04.11 16:04:27 4: sduino2: Parse_MU, Decoded matched MU protocol id 45 dmsg r200005000032000000B289 length 88 dispatch(1/4) RSSI = -74
2021.04.11 16:04:27 3: Unknown Revolt device 2000, please define it

2021.04.11 16:04:46 4: sduino2: Read, msg READredu: MU;P1=10300;P2=-348;P3=112;P4=-254;P5=233;D=1234543434343434343434343434343434343434343434343434343454345454343454345434343454343454345434343454545434343434345454343434343434343434343434343454345454343454345434343454343454345434343454545434343434345450;CP=3;R=0;
2021.04.11 16:04:46 4: sduino2: Parse_MU, Fingerprint for MU protocol id 45 -> Revolt matches, trying to demodulate
2021.04.11 16:04:46 4: sduino2: Parse_MU, Decoded matched MU protocol id 45 dmsg r400000165128E0C000B289 length 88 dispatch(1/4) RSSI = -74
2021.04.11 16:04:46 3: Unknown Revolt device 4000, please define it

Plus some error cases from the 2nd device with my patch blocking the message:

2021.04.21 02:23:38 4: sduino2: Read, msg READredu: MU;P0=-328;P1=143;P2=-218;P5=264;P6=-30920;P7=9988;D=67012521212121212121212121212121212121212121212121212121212121212121212121212121212121252521212521212121212121212121212121212121212121212121212121212521252121212521212121252521252125212121212521212521252521252;CP=1;R=0;
2021.04.21 02:23:38 4: sduino2: Parse_MU, Fingerprint for MU protocol id 45 -> Revolt matches, trying to demodulate
2021.04.21 02:23:38 4: sduino2: Revolt, 103 bits received
2021.04.21 02:23:38 3: sduino2: Revolt, message ignored, checksum mismatch, 208 != 66

2021.04.21 02:27:50 4: sduino2: Read, msg READredu: MU;P0=250;P4=10156;P5=-352;P6=140;P7=-224;D=4567076767676767676767676767676767676767676767676767676767076707676767670707670767076767676707076707670707076767676767676767676767676767676767676767076707676767670707670767076767676707076707670707076760;CP=6;R=0;
2021.04.21 02:27:50 4: sduino2: Parse_MU, Fingerprint for MU protocol id 45 -> Revolt matches, trying to demodulate
2021.04.21 02:27:50 4: sduino2: Revolt, 99 bits received
2021.04.21 02:27:50 3: sduino2: Revolt, message ignored, checksum mismatch, 198 != 53

2021.04.21 06:17:52 4: sduino2: Read, msg READredu: MU;P0=-238;P1=239;P2=122;P6=10140;P7=-368;D=672010202020202020202020202020202020202020202020202020202010201020202010202020201010201020102020202010102010201010102020202020202020202020202020202010201020202010202020201010201020102020202010102010201010102020;CP=2;R=0;
2021.04.21 06:17:52 4: sduino2: Parse_MU, Fingerprint for MU protocol id 45 -> Revolt matches, trying to demodulate
2021.04.21 06:17:52 4: sduino2: Revolt, 104 bits received
2021.04.21 06:17:52 3: sduino2: Revolt, message ignored, checksum mismatch, 92 != 67

I will look into writing testcases later this week.

[gernot-h]

I now tested the patch successfully on a FHEM test instance (using FHEMduino hardware with RXB6 receiver) updated to RFFHEM master branch with valid messages from a real Revolt device and messages with invalid CRC from an Arduino faking Revolt messages. :)

I also slightly improved log output to print full message in case of checksum errors so the (experienced) user can see which message (from which Revolt device -- each message starts with the device id) was dropped.

I also wrote a testcase as you suggested using the 2nd and 4th example from my last comment. I'm however not that fluent in reading results from Github PRs, so please let me know if the test does what it shall do!

Finally, I updated this PR's description and added an entry to CHANGED, so this PR is now ready from my side - thanks in advance for reviewing it!

[gernot-h]

Sorry, fixed the test plan.

[sidey79]

Can you please update $VERSION in the two changed files by 0.01 ?

[gernot-h]

Can you please update $VERSION in the two changed files by 0.01 ?

Sure, done.

[sidey79]

@HomeAutoUser @elektron-bbs

Aus meiner Sicht können wir den PR übernehmen.
In "https://github.com/RFD-FHEM/SIGNALduino_TOOL/blob/33ab9464e3ba54c6d2572764c13c3553d240b7b9/FHEM/lib/SD_Device_ProtocolList.json#L698" brauchen wir ja nichts ergänzen denke ich oder?

[elektron-bbs]

Aus meiner Sicht spricht nichts dagegen. Ob die Daten in dem JSON noch passen, könnte ja @gernot-h selbst prüfen.

[sidey79]

Da wir es automatisch testen ob das dmsg Ergebnis zum soll stimmt, braucht er das aus meiner Sicht nicht prüfen

[gernot-h]

Aus meiner Sicht spricht nichts dagegen. Ob die Daten in dem JSON noch passen, könnte ja @gernot-h selbst prüfen.

Habe inzwischen etwas Übung mit dem dekodieren der Telegramme, daher habe ich mir https://github.com/RFD-FHEM/SIGNALduino_TOOL/blob/33ab9464e3ba54c6d2572764c13c3553d240b7b9/FHEM/lib/SD_Device_ProtocolList.json#L698" auch noch kurz angeschaut, passt exakt zu meinem Kenntnisstand. Und die Checksumme sollte auch passen, mein Patch müsste also damit auch zufrieden sein.