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
Fix of cover-tilt in homekit_controller #91631
Fix of cover-tilt in homekit_controller #91631
Conversation
It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
Hey there @Jc2k, @bdraco, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the tests you will see create_window_covering_service_with_h_tilt
and create_window_covering_service_with_v_tilt
. These have default values for minValue
and maxValue
.
This means that you are only testing the 0.9
cases and NOT the -0.9
case.
You need to duplicate both of those functions and change them for the -0.9 case, then duplicate any tests that involve HORIZONTAL_TILT_TARGET or VERTICAL_TILT_TARGET and make those duplicates also test the -0.9 case.
Right now none of the tests run with a scale of -0.9
so they provide no guarantee's that your change has worked.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Sorry for barging in on this, but just wondering how +90°/-90° tilts are considered in this fix? Will those work too if there are just separate cases for +90° or -90°? (not entirely sure how they are being reported in Home Assistant since I can't tell if mine have been set up correctly by the technicians, so mine still show up as 0 to -90 in the dev tools) |
Not sure what you are asking. From community testing there are 4 cases - horizontal and vertical and -90 to 0 and 0 to 90. We just need a scale factor to go from arc degrees to % and back. That's either -0.9 or 0.9. You either multiply or divide by it, which is what this PR does. |
Thank you. You are right, currently 0.9 is being tested only. To cover tests for -0.9, I think as a non-python-expert, I would need to define the |
No, global variables are typically frowned upon in python and wouldn't pass code review in HA (in general). The simplest solution is what I said above - duplicate the tests. The tests create a mock device with a 0-90 range and we need variants of those tests that create a device with a -90 to 0 range. The 2 functions I called out are where the fake characteristics are defined so that's where you can provide your own variables for minValue and maxValue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix of cover-tilt and its tests
Thank you, you're absolutely right, no need for the global variable. I added additional tests which test against the |
Is there anything still missing from my side for the review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @badewanne1234 👍
When this will be avaliable in HA? |
@mat-sienczyk next release. |
* Fix of cover-tilt in homekit_controller * Fix of cover-tilt and its tests
Proposed change
This change fixes the bug of cover-tilt not working in the component homekit_controller.
Type of change
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: