[Bug] `TargetPosition` of `WindowCovering` shouldn't update while in motion #852

burmistrzak · 2024-04-22T05:35:07Z

Is there an existing issue for this?

I have searched the existing issues

Describe the bug

As far as I understand the HAP specification, we shouldn't update the TargetPosition of WindowCovering accessory while it's still moving. This is especially true when a open/close command came directly from Home.app via user input.

Otherwise, the UX is kind awful and UI elements are quickly jumping around.

All certified accessories I've seen so far, update their TargetPosition only once before they're starting to move.
I suggest we follow their example and adjust the current implementation accordingly.

Related devices

Bosch BMCT-SLZ

Related Devices

No response

Steps To Reproduce

A)

Tap the icon of a WindowCovering service in Home.app to fully open (or close) a covering
Observe the status in the accessory tile
Status switches quickly from Closed to Opening, back to Closed, and finally to 2% Open, 10% Open, etc.

B)

Tap on a WindowCovering service in Home.app to reveal controls
Choose an arbitrary point on the slider and tap it to partially open a covering
The slider quickly moves to the chosen position/percentage and stays there for a moment
Slider suddenly jumps back to previous position
Slider slowly moves towards the set percentage

Expected behavior

A)

Tap the icon of a WindowCovering service in Home.app to e.g. fully open a covering
Status switches to Opening while in motion
Tile status finally updates to x% Open

B)

Tap on a WindowCovering service in Home.app to reveal controls
Choose an arbitrary point on the slider and tap it to partially open a covering
The slider quickly moves to the chosen position/percentage and stays there

Device entry

{
    "date_code": "20231030",
    "definition": {
      "description": "Light/shutter control unit II",
      "exposes": [
        {
          "access": 7,
          "description": "Module controlled by a rocker switch or a button",
          "label": "Switch type",
          "name": "switch_type",
          "property": "switch_type",
          "type": "enum",
          "values": [
            "button",
            "button_key_change",
            "rocker_switch",
            "rocker_switch_key_change"
          ]
        },
        {
          "access": 1,
          "category": "diagnostic",
          "description": "Link quality (signal strength)",
          "label": "Linkquality",
          "name": "linkquality",
          "property": "linkquality",
          "type": "numeric",
          "unit": "lqi",
          "value_max": 255,
          "value_min": 0
        },
        {
          "features": [
            {
              "access": 3,
              "label": "State",
              "name": "state",
              "property": "state",
              "type": "enum",
              "values": [
                "OPEN",
                "CLOSE",
                "STOP"
              ]
            },
            {
              "access": 7,
              "description": "Position of this cover",
              "label": "Position",
              "name": "position",
              "property": "position",
              "type": "numeric",
              "unit": "%",
              "value_max": 100,
              "value_min": 0
            }
          ],
          "type": "cover"
        },
        {
          "access": 1,
          "description": "Shutter motor actual state ",
          "label": "Motor state",
          "name": "motor_state",
          "property": "motor_state",
          "type": "enum",
          "values": [
            "idle",
            "opening",
            "closing"
          ]
        },
        {
          "access": 7,
          "description": "Enable/Disable child lock",
          "label": "Child lock",
          "name": "child_lock",
          "property": "child_lock",
          "type": "binary",
          "value_off": "OFF",
          "value_on": "ON"
        },
        {
          "access": 7,
          "description": "Calibration closing time",
          "endpoint": "closing_time",
          "label": "Calibration",
          "name": "calibration",
          "property": "calibration_closing_time",
          "type": "numeric",
          "unit": "s",
          "value_max": 90,
          "value_min": 1
        },
        {
          "access": 7,
          "description": "Calibration opening time",
          "endpoint": "opening_time",
          "label": "Calibration",
          "name": "calibration",
          "property": "calibration_opening_time",
          "type": "numeric",
          "unit": "s",
          "value_max": 90,
          "value_min": 1
        }
      ],
      "model": "BMCT-SLZ",
      "options": [
        {
          "access": 2,
          "description": "Inverts the cover position, false: open=100,close=0, true: open=0,close=100 (default false).",
          "label": "Invert cover",
          "name": "invert_cover",
          "property": "invert_cover",
          "type": "binary",
          "value_off": false,
          "value_on": true
        },
        {
          "access": 2,
          "description": "State actions will also be published as 'action' when true (default false).",
          "label": "State action",
          "name": "state_action",
          "property": "state_action",
          "type": "binary",
          "value_off": false,
          "value_on": true
        }
      ],
      "supports_ota": false,
      "vendor": "Bosch"
    },
    "disabled": false,
    "endpoints": {
      "1": {
        "bindings": [
          {
            "cluster": "genIdentify",
            "target": {
              "endpoint": 1,
              "ieee_address": "<REDACTED>",
              "type": "endpoint"
            }
          },
          {
            "cluster": "closuresWindowCovering",
            "target": {
              "endpoint": 1,
              "ieee_address": "<REDACTED>",
              "type": "endpoint"
            }
          },
          {
            "cluster": "manuSpecificBosch10",
            "target": {
              "endpoint": 1,
              "ieee_address": "<REDACTED>",
              "type": "endpoint"
            }
          }
        ],
        "clusters": {
          "input": [
            "genBasic",
            "genIdentify",
            "genGroups",
            "genScenes",
            "closuresWindowCovering",
            "seMetering",
            "haElectricalMeasurement",
            "haDiagnostic",
            "manuSpecificBosch10"
          ],
          "output": [
            "genTime",
            "genOta"
          ]
        },
        "configured_reportings": [
          {
            "attribute": "currentPositionLiftPercentage",
            "cluster": "closuresWindowCovering",
            "maximum_report_interval": 65000,
            "minimum_report_interval": 1,
            "reportable_change": 1
          }
        ],
        "scenes": []
      },
      "2": {
        "bindings": [
          {
            "cluster": "genIdentify",
            "target": {
              "endpoint": 1,
              "ieee_address": "<REDACTED>",
              "type": "endpoint"
            }
          },
          {
            "cluster": "genOnOff",
            "target": {
              "endpoint": 1,
              "ieee_address": "<REDACTED>",
              "type": "endpoint"
            }
          }
        ],
        "clusters": {
          "input": [
            "genIdentify",
            "genGroups",
            "genScenes",
            "genOnOff",
            "manuSpecificBosch10"
          ],
          "output": []
        },
        "configured_reportings": [
          {
            "attribute": "onOff",
            "cluster": "genOnOff",
            "maximum_report_interval": 3600,
            "minimum_report_interval": 0,
            "reportable_change": 0
          }
        ],
        "scenes": []
      },
      "3": {
        "bindings": [
          {
            "cluster": "genIdentify",
            "target": {
              "endpoint": 1,
              "ieee_address": "<REDACTED>",
              "type": "endpoint"
            }
          },
          {
            "cluster": "genOnOff",
            "target": {
              "endpoint": 1,
              "ieee_address": "<REDACTED>",
              "type": "endpoint"
            }
          }
        ],
        "clusters": {
          "input": [
            "genIdentify",
            "genGroups",
            "genScenes",
            "genOnOff",
            "manuSpecificBosch10"
          ],
          "output": []
        },
        "configured_reportings": [
          {
            "attribute": "onOff",
            "cluster": "genOnOff",
            "maximum_report_interval": 3600,
            "minimum_report_interval": 0,
            "reportable_change": 0
          }
        ],
        "scenes": []
      }
    },
    "friendly_name": "<REDACTED>",
    "ieee_address": "<REDACTED>",
    "interview_completed": true,
    "interviewing": false,
    "manufacturer": "Bosch",
    "model_id": "RBSH-MMS-ZB-EU",
    "network_address": <REDACTED>,
    "power_source": "Mains (single phase)",
    "software_build_id": "2.16.00",
    "supported": true,
    "type": "Router"
  }

Status update

{
  "calibration_closing_time": 10,
  "calibration_opening_time": 10,
  "child_lock": "OFF",
  "child_lock_left": "OFF",
  "child_lock_right": "OFF",
  "device_mode": "shutter",
  "last_seen": "2024-04-22T07:11:08+02:00",
  "linkquality": 98,
  "motor_state": "idle",
  "position": 0,
  "state": "CLOSE",
  "state_left": "OFF",
  "state_right": "OFF",
  "switch_type": "button"
}

Messages from this plugin

No response

This plugin

1.11.0-beta.3

Homebridge

1.7.0

Zigbee2MQTT

1.36.1-dev

Homebridge Config UI X (if applicable)

No response

The text was updated successfully, but these errors were encountered:

itavero · 2024-04-22T07:21:17Z

Previously I added this so that but actually showed that something was happening. Perhaps they changed the Home.app since the initial implementation though.

Do you have an "official" HomeKit window covering so we can peek at when they update their states?

burmistrzak · 2024-04-22T17:26:05Z

Do you have an "official" HomeKit window covering so we can peek at when they update their states?

Sure! Here are some examples:

Bosch Shutter Control Gen. I (868 MHz, HAP over IP via certified bridge)
Bosch Shutter Control Gen. II (Zigbee, HAP over IP via certified bridge)

Both only seem to update their current position once stopped. They also don't support HoldPosition.

Eve MotionBlinds (Matter over Thread, previously HAP over Thread)

This one is interesting. It supports HoldPosition and actually does update its CurrentPosition while in motion!

However, I now know what's actually going on here. Give me a second to verify... ⚙️

Edit: Below are all position values reported by a Bosch BMCT-SLZ when fully closing a covering from HomeKit. Latest values on top.

burmistrzak · 2024-04-22T17:36:54Z

@itavero Ok, here's what's going on:

We're continually updating TargetPosition alongside CurrentPosition.
That's unfortunately incorrect. TargetPosition should only be set once, ideally before the covering starts moving.

I can confirm that all of the aforementioned accessories behave in exactly this manner, that is, writing TargetPosition only once per movement.

burmistrzak · 2024-04-23T18:13:25Z

@itavero Did some further testing, and logged HAP commands coming from a Bosch Shutter Control Gen. I when using the in-wall switch directly.

a.

PositionState changes from STOPPED to DECREASING
TargetPosition is set to 0%
[...]
PositionState changes from DECREASING to STOPPED
CurrentPosition is set to 63%
TargetPosition is set to 63%

And fully open from HomeKit:

b.

TargetPosition is set to 100%
PositionState changes from STOPPED to INCREASING
[...]
PositionState changes from INCREASING to STOPPED
CurrentPosition is set to 100%

‼️ Key takeaway here's that TargetPosition should only be set more than once in case a covering gets STOPPED before reaching the initial TargetPosition (see a.2. != a.5.), otherwise Home.app gets confused.

itavero · 2024-04-26T09:40:57Z

Thanks for looking into this. I did not have time to process all your information, but I can see indeed some opportunities for improvements.

I'm not sure when I'll have time to dive into this myself as well, but I'm open to accepting a PR to improve this behavior.

burmistrzak · 2024-04-26T16:21:42Z

Thanks for looking into this. I did not have time to process all your information, but I can see indeed some opportunities for improvements.

No worries, it's indeed a lot. Let me quickly summarize my key findings for you:

TargetPosition is always updated alongside CurrentPosition.
Updating CurrentPosition during movement is fine, as long as TargetPosition is only set once at the beginning.
TargetPosition and CurrentPosition must be set, together alongside PositionState.STOPPED, in case the covering gets abruptly stopped (e.g. by in-wall switch) before reaching the initial target position.
Setting PositionState correctly, is more important than constant CurrentPosition updates.

I'm not sure when I'll have time to dive into this myself as well, but I'm open to accepting a PR to improve this behavior.

I'm happy to provide a PR, but probably will need a little bit of support, because I'm not yet super familiar with the codebase. 😊
Please let me know, if you have any suggestions to get me started.

burmistrzak · 2024-04-28T03:37:58Z

@itavero I did some more in depth analysis of the plugin's current behavior with the Bosch BCMT-SLZ, and here's what I've found:

The set target position is not ignored. See the corresponding logs here

homebridge-z2m/src/converters/cover.ts

Lines 180 to 185 in c02eb2d

    
           const doIgnoreIfEqual = this.ignoreNextUpdateIfEqualToTarget; 
        
           this.ignoreNextUpdateIfEqualToTarget = false; 
        
           if (latestPosition === this.lastPositionSet && doIgnoreIfEqual) { 
        
             this.accessory.log.debug(`${this.accessory.displayName}: cover: ignore position update (equal to last target)`); 
        
             return; 
        
           }

When changing the target position from either Home.app or an in-wall switch, the state during movement is always assumed to be isStopped. Corresponding logs here and here

homebridge-z2m/src/converters/cover.ts

Lines 257 to 263 in c02eb2d

    
           if (isStopped) { 
        
             // Update target position and position state 
        
             // This should improve the UX in the Home.app 
        
             this.accessory.log.debug(`${this.accessory.displayName}: cover: assume movement stopped`); 
        
             this.service.updateCharacteristic(hap.Characteristic.PositionState, hap.Characteristic.PositionState.STOPPED); 
        
             this.service.updateCharacteristic(hap.Characteristic.TargetPosition, characteristicValue); 
        
           }

PositionState is not updated when controlled from outside of HomeKit, e.g. in-wall switch or MQTT.

burmistrzak · 2024-04-30T04:19:03Z

Previously I added this so that but actually showed that something was happening. Perhaps they changed the Home.app since the initial implementation though.

Looks to be the case. 😬
Home.app on iOS seems to be a bit more tolerant, but on macOS, the UX is even worse.

The sooner we can get WindowCovering into compliance, the less issues will crop up with every new release.

Also, what's the deal with that updateTimer thingy? Why are we manually polling for updates when there's reporting? 🤔

burmistrzak · 2024-05-01T01:25:40Z

@itavero I have an update for you. 😎

The Good: I have a 99% HAP-compliant WindowCovering implementation.

The Bad: It requires motor_state to be exposed.

There's unfortunately no way around it. We need some kind of "direction of movement" indicator from the device itself , otherwise we're just guessing based on position alone, and that's not good enough.

By default, your original code is used for all devices, unless motor_state is detected, then my stuff takes over.
I'll publish a gist of the compiled JS asap, and update #854 with a proper TypeScript implementation later. Cool?

burmistrzak · 2024-05-06T18:56:31Z

@itavero So, #854 is ready for review! 🤓

itavero · 2024-05-06T18:57:59Z

I hope I'll have some time in the next week or so to review it. Thanks!

burmistrzak · 2024-06-30T20:26:14Z

Yey! Thanks for merging! 🥳

burmistrzak added the bug Something isn't working label Apr 22, 2024

burmistrzak changed the title ~~[Bug] CurrentPosition of WindowCovering shouldn't update while in motion~~ [Bug] TargetPosition of WindowCovering shouldn't update while in motion Apr 22, 2024

This comment was marked as outdated.

Sign in to view

burmistrzak mentioned this issue Apr 25, 2024

Add meta option to disable return value on /set for cover_position_tilt Koenkk/zigbee-herdsman-converters#7438

Merged

This comment was marked as outdated.

Sign in to view

burmistrzak mentioned this issue Apr 29, 2024

Improve UX of WindowCovering by using motor_state #854

Merged

itavero closed this as completed in #854 Jun 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug] `TargetPosition` of `WindowCovering` shouldn't update while in motion #852

[Bug] `TargetPosition` of `WindowCovering` shouldn't update while in motion #852

burmistrzak commented Apr 22, 2024 •

edited

Loading

itavero commented Apr 22, 2024

burmistrzak commented Apr 22, 2024 •

edited

Loading

burmistrzak commented Apr 22, 2024 •

edited

Loading

burmistrzak commented Apr 23, 2024

This comment was marked as outdated.

itavero commented Apr 26, 2024

burmistrzak commented Apr 26, 2024 •

edited

Loading

This comment was marked as outdated.

burmistrzak commented Apr 28, 2024

burmistrzak commented Apr 30, 2024

burmistrzak commented May 1, 2024

burmistrzak commented May 6, 2024

itavero commented May 6, 2024

burmistrzak commented Jun 30, 2024

[Bug] TargetPosition of WindowCovering shouldn't update while in motion #852

[Bug] TargetPosition of WindowCovering shouldn't update while in motion #852

Comments

burmistrzak commented Apr 22, 2024 • edited Loading

Is there an existing issue for this?

Describe the bug

Related devices

Related Devices

Steps To Reproduce

Expected behavior

Device entry

Status update

Messages from this plugin

This plugin

Homebridge

Zigbee2MQTT

Homebridge Config UI X (if applicable)

itavero commented Apr 22, 2024

burmistrzak commented Apr 22, 2024 • edited Loading

burmistrzak commented Apr 22, 2024 • edited Loading

burmistrzak commented Apr 23, 2024

This comment was marked as outdated.

itavero commented Apr 26, 2024

burmistrzak commented Apr 26, 2024 • edited Loading

This comment was marked as outdated.

burmistrzak commented Apr 28, 2024

burmistrzak commented Apr 30, 2024

burmistrzak commented May 1, 2024

burmistrzak commented May 6, 2024

itavero commented May 6, 2024

burmistrzak commented Jun 30, 2024

[Bug] `TargetPosition` of `WindowCovering` shouldn't update while in motion #852

[Bug] `TargetPosition` of `WindowCovering` shouldn't update while in motion #852

burmistrzak commented Apr 22, 2024 •

edited

Loading

burmistrzak commented Apr 22, 2024 •

edited

Loading

burmistrzak commented Apr 22, 2024 •

edited

Loading

burmistrzak commented Apr 26, 2024 •

edited

Loading