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

FSK check msg length #959

Merged
merged 4 commits into from
May 15, 2021
Merged

FSK check msg length #959

merged 4 commits into from
May 15, 2021

Conversation

elektron-bbs
Copy link
Contributor

  • 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, ...)
    feature

  • What is the current behavior? (You can also link to an open issue here)
    only check min lenght of FSK messages

  • What is the new behavior (if this is a feature change)?
    check min and max lenght of FSK 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:

elektron-bbs and others added 2 commits May 5, 2021 20:58
before: check only min message length
after: chek min and max message lenght
@codecov
Copy link

codecov bot commented May 5, 2021

Codecov Report

Merging #959 (cf3cc13) into master (f1b0729) will decrease coverage by 0.00%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #959      +/-   ##
==========================================
- Coverage   58.33%   58.32%   -0.01%     
==========================================
  Files         106      106              
  Lines        8448     8447       -1     
  Branches     1315     1315              
==========================================
- Hits         4928     4927       -1     
  Misses       2634     2634              
  Partials      886      886              
Flag Coverage Δ
fhem 48.50% <75.00%> (-0.01%) ⬇️
modules 58.32% <75.00%> (-0.01%) ⬇️
perl 91.52% <ø> (ø)
unittests 58.32% <75.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
FHEM/00_SIGNALduino.pm 62.05% <75.00%> (-0.02%) ⬇️

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 f1b0729...cf3cc13. Read the comment docs.

@sidey79
Copy link
Contributor

sidey79 commented May 5, 2021

@elektron-bbs

Es könnte sein, dass wir uns mal dazu entscheiden hatten die maximallänge erst nach dem Aufruf der methode zu prüfen.
Die Mindestlänge ist ja quasi das mindeste. Die Länge könnte sich aber nach dem Durchlauf der Methode verändern.

Wie denkst Du darüber, denn in Range erst nach dem finalen Verarbeiten zu prüfen?

@elektron-bbs
Copy link
Contributor Author

Das war für mich schon immer irreführend, weil es je nach Nachrichtentyp (MS, MU, MC und jetzt noch MN) unterschiedlich gehandhabt wird.
M.E. sollten solche grundlegenden Prüfungen zuerst erfolgen, bevor irgendwelche Manipulationen an den Nachrichten durchgeführt werden. Warum sollte ich z.B. die Nachricht erst durch eine CRC-Routine o.ä. jagen, wenn vorher schon feststeht das sie zu kurz oder zu lang ist?

@sidey79
Copy link
Contributor

sidey79 commented May 5, 2021

@elektron-bbs

Bei MU und MC war der Grund folgender:

Es könnte sein, dass mehr als eine Übertragung in den Daten steckt.
Eine Prüfung auf die Maximallänge ist nicht sehr sinnvoll, da ja ggf. zwei oder drei Wiederholungen enthalten sind.
Daher wird hier erst nach einer Verarbeitung der Signale etwas geprüft.

Kann es dieses Verhalten bei den MN daten auch geben oder ist das ausgeschlossen?

@elektron-bbs
Copy link
Contributor Author

elektron-bbs commented May 6, 2021

Die Nachrichtenlänge ist abhängig von Register 0x03 FIFOTHR:

"Set the threshold for the TX FIFO and RX FIFO. The threshold is exceeded when the number of bytes in the FIFO is equal to or higher than the threshold value."

Das bestimmt allerdings nur die minimale Länge. Ich habe hier z.B. folgende Beobachtung gemacht:

Lacrosse_mode1 - FIFO_THR 8 Byte
--------------------------------
SIGNALESP ESP8266: 9205566A7CAAAA00       8 Byte
SIGNALduino Nano:  9205566A7CAAAA000086  10 Byte

Bresser_5in1 - FIFO_THR 28 Byte
-------------------------------
SIGNALESP ESP8266: E7527FF7AFF7EFDDFEB7DBC87F18AD80085008102201482437800002    28 Byte
SIGNALduino Nano:  E7527FF7AFF7EFDDFEB7DBC87F18AD8008500810220148243780000085  29 Byte

Es scheint so, als ob der langsamere Prozessor die Daten etwas verzögert abholt. Das bedeutet, das wir die maximale Länge lieber immer etwas größer definieren sollten.
Ob Wiederholungen in den Nachrichten vorkommen können, kann ich nicht beantworten. Bei den bisher von uns eingebauten Protokollen ist das nicht der Fall.
Wenn keine length_max definiert ist, erfolgt auch keine Prüfung darauf. Dann wird jede Nachricht erst noch mehr oder weniger aufwändig weiter verarbeitet.

@elektron-bbs elektron-bbs merged commit e7d0455 into master May 15, 2021
@elektron-bbs elektron-bbs deleted the master_checkMsgLength branch June 16, 2021 19:18
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.

3 participants