Improve UX of WindowCovering by using motor_state
#854
Conversation
|
@itavero What about utilizing So we're only updating All other devices however, would be stuck with the current implementation. Another relevant PR has been merged: Koenkk/zigbee-herdsman-converters#7470 |
burmistrzak
changed the title
WindowCovering with Bosch BCMT-SLZWindowCovering by using motor_state
|
@itavero 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. |
Don't worry. 😊 |
|
@itavero Would appreciate if you can already run the two workflows, so I can make any necessary changes in due time. ✌️ |
Codecov ReportAttention: Patch coverage is
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 659 -133
==========================================
- Hits 1921 1633 -288
+ Misses 883 747 -136
- Partials 99 170 +71
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@itavero Alright, I've completely reworked how we're checking for the presence of |
| CoverHandler.generateIdentifier(m.endpoint) === CoverHandler.generateIdentifier(e.endpoint) && | ||
| m.type === ExposesKnownTypes.ENUM && | ||
| m.name === 'motor_state' && | ||
| exposesHasEnumProperty(m) && |
There was a problem hiding this comment.
Should also check if it is published by Zigbee2MQTT.
| exposesHasEnumProperty(m) && | |
| exposesIsPublished(m) && | |
| exposesHasEnumProperty(m) && |
|
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. |
|
@itavero Implemented the changes you've suggested. Based on my testing, everything still works! 😅 |
|
@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. 🛫 |
There was a problem hiding this comment.
I had a glance at the other changes and they seem fine. Hopefully I'll have time to do a proper review in the next few days and get this thing merged.
Sonarcloud is also complaining about the complexity of scaleAndUpdateCurrentPosition, but I think I can live with that for now.
|
@itavero Alright, our robot overlords 🤖 should now be pleased. 😅 |
|
@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. 🤞 |
|
@itavero That's alright. Having a good workplace setup is quite important. 👌 |
|
@itavero Are there any blocking issues you want me to take a look at? |
|
Sorry. Haven't gotten around to it yet. |
Would be great timing! |
|
@itavero Do you think we'll be able to finish this up before tomorrow's Zigbee2MQTT release? 🤞 |
| switch (this.motorState) { | ||
| case CoverHandler.MOTOR_STATE_CLOSING: | ||
| newPositionState = hap.Characteristic.PositionState.DECREASING; | ||
| newTargetPosition = 0; |
There was a problem hiding this comment.
@burmistrzak Just wondering, did you also check the UX by setting a target position from the app to a value less than 100 or greater than 0?
I'm curious if these fixed values of 0 and 100 for newTargetPosition are okay, or if they may also lead to weird behavior.
There was a problem hiding this comment.
I'll merge it for now. We can always improve it later, if needed.
There was a problem hiding this comment.
@itavero Well, that's how certified accessories do it. 😅
We only update the target position when the command is not coming from HomeKit (e.g. a physical switch, etc.), because the HAP controller doesn't already know about the new target.
So in case of a physical switch, setting the max value makes total sense, IMHO.
However, when e.g. MQTT is used to set a new target position, we can't really distinguish between a new target position and the current position. That's a quirk of Zigbee2MQTT, that I might have to address upstream.
|
|
🆗 Published SonarCloud and code coverage data. |
|
Part of release 1.11.0-beta.6 |
Implements support for the optional
motor_stateattribute 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