Improve UX of `WindowCovering` by using `motor_state`

burmistrzak commented 2 months ago

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 commented 2 months ago

@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: https://github.com/Koenkk/zigbee-herdsman-converters/pull/7470

burmistrzak commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

codecov[bot] commented 1 month ago

Codecov Report

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

Project coverage is 64.03%. Comparing base (31b0fd9) to head (62fb214). Report is 12 commits behind head on master.

:exclamation: Current head 62fb214 differs from pull request most recent head d81fb3c

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

Files	Patch %	Lines
src/converters/cover.ts	91.22%	5 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #854 +/- ## ========================================== - Coverage 66.17% 64.03% -2.14% ========================================== Files 40 40 Lines 2903 2550 -353 Branches 792 659 -133 ========================================== - Hits 1921 1633 -288 + Misses 883 747 -136 - Partials 99 170 +71 ``` | [Flag](https://app.codecov.io/gh/itavero/homebridge-z2m/pull/854/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Arno+Moonen) | Coverage Δ | | |---|---|---| | [tests](https://app.codecov.io/gh/itavero/homebridge-z2m/pull/854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Arno+Moonen) | `64.03% <91.22%> (-2.14%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Arno+Moonen#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

burmistrzak commented 1 month ago

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

itavero commented 1 month ago

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.

burmistrzak commented 1 month ago

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

burmistrzak commented 1 month ago

@itavero I've been dog-fooding this PR for the past few days, and it's working quite nicely. So from my side, we should be ready for takeoff. 🛫

burmistrzak commented 1 month ago

@itavero Alright, our robot overlords 🤖 should now be pleased. 😅

itavero commented 3 weeks ago

@burmistrzak Sorry that I didn't get around to merging and releasing this. I've been busy moving my home office to another room, which is taking a bit more time than I hoped. I will try to wrap this PR up in the coming week. 🤞

burmistrzak commented 3 weeks ago

@itavero That's alright. Having a good workplace setup is quite important. 👌

burmistrzak commented 2 weeks ago

@itavero Are there any blocking issues you want me to take a look at? Would be great if we can get this out the door before the next Zigbee2MQTT release, because I still need some time to validate recent changes to the relevant Bosch converters. 😊

itavero commented 2 weeks ago

Sorry. Haven't gotten around to it yet. I have some time off from work next week, so I hope to merge and release it during that time or before.

burmistrzak commented 2 weeks ago