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

xFSK - cc110x commands & commandref #990

Merged
merged 54 commits into from
Aug 2, 2021

Conversation

HomeAutoUser
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
    -> docs update

  • What is the current behavior? (You can also link to an open issue here)
    -> no possibility of direct adjustment
    -> superfluous display at OOK in Reading cc1101_config_ext (Syncmod is only xFSK)

  • What is the new behavior (if this is a feature change)?
    -> new set commands at xFSK - deviatn & dataRate (direct setting option | better handling for users)
    -> addition to commandref

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

  • Other information:

HomeAutoUser and others added 29 commits December 30, 2019 16:34
…ogo82148/actions-setup-perl-v1.10.0

Bump shogo82148/actions-setup-perl from v1.9.7 to v1.10.0
 * sort cc1101 sets alphabetical
 * revised hardtabs for Perl::Critic
 * added cc1101_dataRate to setlist
 * added cc1101_deviatn to setlist
 * revised output DataRate to kBaud
 * revised spelling mistake Bandwidth kHz
 * revised commandref
 * revised test with new data
@codecov
Copy link

codecov bot commented Jul 12, 2021

Codecov Report

Merging #990 (56e04d8) into master (8dc6ae9) will increase coverage by 0.22%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #990      +/-   ##
==========================================
+ Coverage   58.75%   58.97%   +0.22%     
==========================================
  Files         109      110       +1     
  Lines        8636     8710      +74     
  Branches     1351     1364      +13     
==========================================
+ Hits         5074     5137      +63     
+ Misses       2638     2635       -3     
- Partials      924      938      +14     
Flag Coverage Δ
fhem 49.22% <78.57%> (+0.36%) ⬆️
modules 58.97% <78.57%> (+0.22%) ⬆️
perl 91.57% <ø> (ø)
unittests 58.97% <78.57%> (+0.22%) ⬆️

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

Impacted Files Coverage Δ
...0_SIGNALduino/02_SIGNALduino_CheckccConfResponse.t 100.00% <ø> (ø)
FHEM/00_SIGNALduino.pm 62.75% <77.21%> (+0.75%) ⬆️
t/FHEM/cc1101/01_func.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 8dc6ae9...56e04d8. Read the comment docs.

 * revised some input data
@HomeAutoUser
Copy link
Contributor Author

Der Test CalcDataRate muss noch überarbeitet werden, da stimmt entweder das Ergebnis oder das angenommene Ergebnis nicht.

Ich habe mal den Test überarbeitet :-)

@HomeAutoUser HomeAutoUser requested a review from sidey79 July 28, 2021 08:42
item '7a';
end();
}, q[verify return values]);
is(cc1101::CalcDataRate($targetHash,qw(57 150)),qw(5c 7a), q[verify return values]);
Copy link
Contributor

@sidey79 sidey79 Jul 28, 2021

Choose a reason for hiding this comment

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

Warum hast Du das auf eine Liste angepasst und nicht bei dem test2 Array Check belassen?

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 machte das weil es Fehler brachte und so nicht. So stimmt das Ergebnis mit dem Input. Getestet auch mit anderen Werte.

Ist das Ergebnis ein Array wirklich?

Copy link
Contributor

Choose a reason for hiding this comment

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

Da bin ich mir recht sicher.

Der Grund, wieso ich den array Check verwend ist, dass jedes Element geprüft werden soll.

Ändert man die Zeile z.B. auf:
is(cc1101::CalcDataRate($targetHash,qw(57 150)),qw(5d 7a), q[verify return values]); würden wir erwarten, dass der Test Fehl schlägt.
Macht er aber nicht, denn das letzte Element im Array passt.

Ich drehe es wieder zurück

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wieso erhalte ich bei der Prüfung auf das Array als Vergleich beim Test immer den letzten Wert? Wenn ich auf Liste prüfe, so erhalte ich beide Werte als Vergleich. Wo denkst du, ist der Fehler hier? Die Sub arbeitet richtig.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sidey79 lange habe ich gesucht und gelesen / getestet. So mit der Variante prüfen wir jeden Wert ob dies stimmt.

sidey79 and others added 4 commits July 28, 2021 23:18
 - Revert checks back to array builder
 * correction tests for comparison of all contents
is([$ret[0], $ret[1]], array {
item '5c';
item '7a';
etc();
Copy link
Contributor

Choose a reason for hiding this comment

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

Verblüffend, wieso das mit dem array vergleich nur so klappt.
Liegt das irgendwie an der ungeschickten Rückgabe aus der Sub?

Bist Du dir sicher, dass wir hier etc() brauchen?
Das würde beliebige weitere Rückgabewerte erlauben.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Verblüffend, wieso das mit dem array vergleich nur so klappt.

Ja ich war auch sehr verblüffend und fast vor dem Wahnsinn.

Liegt das irgendwie an der ungeschickten Rückgabe aus der Sub?

Ich denke eher, das liegt an dem Test2:: was dies nicht richtig umsetzt.
Dazu fehlt mir aber von allen Sachen der Fernblick.

Bist Du dir sicher, dass wir hier etc() brauchen?

Ich habe es drin gelassen weil ich so alle Fälle mit unterschiedlichen Werten getestet hatte.
Gern kann es auch raus bei Bedarf.

@sidey79
Copy link
Contributor

sidey79 commented Jul 29, 2021

Jetzt haben wir das Problem, dass der Pull Request aus deinem Repository kommt und dadurch mit deinen Rechten ausgeführt wird.
Dadurch kann der VersionReplace job nicht in den Pull Request ein Update machen und der job schlägt fehl

@HomeAutoUser
Copy link
Contributor Author

Das konnte ich mir schon denken.
Der einfachste Weg ist, wenn wir uns mit dem Code einig sind so überführe ich die Änderungen in einen Branch auf RFHEM oder hast du eine andere Lösung bzw. Idee?

@sidey79
Copy link
Contributor

sidey79 commented Jul 30, 2021

Zu viele Baustellen parallel gerade :(

@HomeAutoUser
Copy link
Contributor Author

Eigendlich ist es ja einfacher, wenn man in dem Fall auf die Aktualisierung des VersionsStrings "verzichtet" weil wir umgehend danach eh andere Anpassung nachreichen im richtigen RePo. (eine Winzige hätte ich auch parat)

@sidey79
Copy link
Contributor

sidey79 commented Aug 2, 2021

@HomeAutoUser

Wie genau meinst Du das mit dem verzichten und nachreichen?

@HomeAutoUser
Copy link
Contributor Author

Ich meinte das so, das in diesem Branch ggf der String nicht aktualisiert wird wegen den Rechten und für die Zukunft muss ich Branchs auf RFFHEM erstellen für die nötigen Rechte. Aber vielleicht geht es ja gleich. Warst ja gerade aktiv ;-)

@sidey79
Copy link
Contributor

sidey79 commented Aug 2, 2021

ja das können wir so machen

t/FHEM/cc1101/01_func.t Show resolved Hide resolved
@HomeAutoUser
Copy link
Contributor Author

Da ändert man etwas was du markiert hast und schon gehen deine Tests nicht mehr grr.
Back to root und nun schauen wieso.

@HomeAutoUser HomeAutoUser merged commit baf8c69 into RFD-FHEM:master Aug 2, 2021
@HomeAutoUser HomeAutoUser deleted the master_cc110x_commands branch August 2, 2021 21:56
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