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

Improve UX of WindowCovering by using motor_state #854

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

burmistrzak
Copy link
Contributor

@burmistrzak burmistrzak commented Apr 28, 2024

Implements support for the optional motor_state attribute exposed by some coverings, while keeping the current behavior for all others.

📌 Unfortunately, it's not really feasible to improve HAP compliance for devices that do not support motor_state, without removing features.

Fixes #852

@burmistrzak burmistrzak marked this pull request as draft April 29, 2024 02:48
@burmistrzak
Copy link
Contributor Author

burmistrzak commented Apr 29, 2024

@itavero What about utilizing motor_state for PositionState on devices that support it?

So we're only updating CurrentPosition when motor_state != idle, and in cases where the command didn't come from Home.app, we just set TargetPosition to the corresponding max value, i.e. opening => 0; closing => 100 because we'll set the final target value anyways, once PositionState is STOPPED.

All other devices however, would be stuck with the current implementation.

Another relevant PR has been merged: Koenkk/zigbee-herdsman-converters#7470

@burmistrzak burmistrzak changed the title Improve UX of WindowCovering with Bosch BCMT-SLZ Improve UX of WindowCovering by using motor_state May 6, 2024
@burmistrzak burmistrzak marked this pull request as ready for review May 6, 2024 18:53
@burmistrzak
Copy link
Contributor Author

@itavero Did you already had time to take a look at the PR?
I personally would have preferred to check for motor_state like you did with position and tilt, but unfortunately motor_state is not part of features...

@itavero
Copy link
Owner

itavero commented May 14, 2024

Did you already had time to take a look at the PR?

Unfortunately I have not had time. Hopefully I can find a moment this week to do at least a quick check, but my schedule is still quite busy.

@burmistrzak
Copy link
Contributor Author

Unfortunately I have not had time. Hopefully I can find a moment this week to do at least a quick check, but my schedule is still quite busy.

Don't worry. 😊
Just let me know if I can help out somehow!

@burmistrzak
Copy link
Contributor Author

@itavero Would appreciate if you can already run the two workflows, so I can make any necessary changes in due time. ✌️

src/converters/cover.ts Outdated Show resolved Hide resolved
src/converters/cover.ts Outdated Show resolved Hide resolved
Copy link

codecov bot commented May 22, 2024

Codecov Report

Attention: Patch coverage is 91.22807% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 64.03%. Comparing base (31b0fd9) to head (b83009d).

Current head b83009d differs from pull request most recent head 78d2848

Please upload reports for the commit 78d2848 to get more accurate results.

Files Patch % Lines
src/converters/cover.ts 91.22% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #854      +/-   ##
==========================================
- Coverage   66.17%   64.03%   -2.14%     
==========================================
  Files          40       40              
  Lines        2903     2550     -353     
  Branches      792      619     -173     
==========================================
- Hits         1921     1633     -288     
- Misses        883      910      +27     
+ Partials       99        7      -92     
Flag Coverage Δ
tests 64.03% <91.22%> (-2.14%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@burmistrzak burmistrzak requested a review from itavero May 22, 2024 01:59
src/converters/cover.ts Outdated Show resolved Hide resolved
@burmistrzak
Copy link
Contributor Author

@itavero Alright, I've completely reworked how we're checking for the presence of motor_state, should now also work with multiple endpoints. 😅

src/converters/cover.ts Outdated Show resolved Hide resolved
src/converters/cover.ts Outdated Show resolved Hide resolved
src/converters/cover.ts Outdated Show resolved Hide resolved
src/converters/cover.ts Show resolved Hide resolved
src/converters/cover.ts Outdated Show resolved Hide resolved
src/converters/cover.ts Outdated Show resolved Hide resolved
src/converters/cover.ts Outdated Show resolved Hide resolved
src/converters/cover.ts Outdated Show resolved Hide resolved
@itavero
Copy link
Owner

itavero commented May 23, 2024

Should probably also mention this in the changelog and the docs for the window covering, but I can also do that before merging the code.

Copy link

sonarcloud bot commented May 25, 2024

Copy link
Contributor

🆗 Published SonarCloud and code coverage data.

@burmistrzak
Copy link
Contributor Author

@itavero Implemented the changes you've suggested. Based on my testing, everything still works! 😅

@burmistrzak burmistrzak requested a review from itavero May 25, 2024 19:53
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.

[Bug] TargetPosition of WindowCovering shouldn't update while in motion
2 participants