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

Improvement of percent calculation #72

Merged
merged 7 commits into from
Aug 7, 2021
Merged

Conversation

Nicks57
Copy link
Contributor

@Nicks57 Nicks57 commented Aug 3, 2021

Improvement of percent calculation in to_percent() for devices that do not return exactly 51200 as final position when closed.

Fixes #47

Improvement of percent calculation in to_percent() for devices that do not return exactly 51200 as final position when closed.
@Julius2342
Copy link
Owner

Thank you :) May I also ask you to add some unit tests (to catch esp edge cases around the maximum and mimimum values.)

You could test e.g. raw values for: 0,1,3,100,198,199,200.

Another question, don't we have to update the from_percent function as well?

@Nicks57
Copy link
Contributor Author

Nicks57 commented Aug 3, 2021

Your welcome :-)

I assume from_percent is used to convert an input command received (e.g. from Home Assistant) to raw (byte array) to send it to KLF200. Since this command will always be accurate (0%-100% i.e. 0-51200) there is no need to add tolerance. The tolerance is only here to compensate the response of Velux Motor that drift slightly from the set_position command.

Can you please advise me how to do and publish the unit tests?

Thank you!

@Julius2342
Copy link
Owner

Julius2342 commented Aug 4, 2021

An example for a unit test you can see here: https://github.com/Julius2342/pyvlx/blob/master/test/alias_array_test.py

It tests the function on a low level (and ensures that the functionality stays the same even after refactorings which happen in the future)

@Julius2342
Copy link
Owner

Hey @Nicks57 , i added the github integration for an automatic execution of unit tests. May I ask you to rebase or merge master?

@Nicks57
Copy link
Contributor Author

Nicks57 commented Aug 7, 2021

Hi Julius!
I merged the master in my fork and added unit test file parameter_test.py. Are the tests ok?

@Julius2342
Copy link
Owner

Julius2342 commented Aug 7, 2021

Please run isort test/parameter_test.py . And pyvlx.exception.PyVLXException has to be removed.

The rest looks fine ...

@Nicks57
Copy link
Contributor Author

Nicks57 commented Aug 7, 2021

Done! Thank you for your help.

@Julius2342 Julius2342 merged commit ab09b0c into Julius2342:master Aug 7, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #72 (3aca264) into master (869029f) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #72   +/-   ##
=======================================
  Coverage   81.79%   81.79%           
=======================================
  Files          73       73           
  Lines        2967     2967           
=======================================
  Hits         2427     2427           
  Misses        540      540           
Impacted Files Coverage Δ
pyvlx/parameter.py 85.93% <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 869029f...3aca264. Read the comment docs.

@Julius2342
Copy link
Owner

Thank you @Nicks57 :)

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.

Window opening % reported as 1 instead of 0 when closed (new vasistas)
3 participants