search

itavero / homebridge-z2m

Expose your Zigbee devices to HomeKit with ease, by integrating 🐝 Zigbee2MQTT with 🏠 Homebridge.
https://z2m.dev
Apache License 2.0
297 stars 47 forks source link

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

Closed burmistrzak closed 4 days ago

burmistrzak commented 2 months ago

Is there an existing issue for this?

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

Related Devices

No response

Steps To Reproduce

A)

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

B)

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

Expected behavior

A)

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

B)

  1. Tap on a WindowCovering service in Home.app to reveal controls
  2. Choose an arbitrary point on the slider and tap it to partially open a covering
  3. 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

itavero commented 2 months ago

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

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

Sure! Here are some examples:

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

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.

0
0
0
9
19
29
39
49
59
69
79
89
98
99
99
0
100
burmistrzak commented 2 months ago

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

@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.

  1. PositionState changes from STOPPED to DECREASING
  2. TargetPosition is set to 0%
  3. [...]
  4. PositionState changes from DECREASING to STOPPED
  5. CurrentPosition is set to 63%
  6. TargetPosition is set to 63%

And fully open from HomeKit:

b.

  1. TargetPosition is set to 100%
  2. PositionState changes from STOPPED to INCREASING
  3. [...]
  4. PositionState changes from INCREASING to STOPPED
  5. 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.

burmistrzak commented 2 months ago

Hmm, I went through the code, but I'm a bit unsure about where these TargetPosition updates are actually coming from. AFAICT, only CurrentPosition should receive periodic updates... 🤔

Edit: After adding some non-invasive logging (to print key functions and their arguments in cover.ts), I saw the following:

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 100
homebridge-1  | this.positionCurrent: -1
homebridge-1  | scaleAndUpdateCurrentPosition: 100
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 0
homebridge-1  | this.positionCurrent: -1
homebridge-1  | scaleAndUpdateCurrentPosition: 0
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 100
homebridge-1  | this.positionCurrent: 100
homebridge-1  | scaleAndUpdateCurrentPosition: 100
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 0
homebridge-1  | this.positionCurrent: 0
homebridge-1  | scaleAndUpdateCurrentPosition: 0
homebridge-1  | isStopped: true
homebridge-1  | handleSetTargetPosition: 100
homebridge-1  | startOrRestartUpdateTimer

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 0
homebridge-1  | this.positionCurrent: 0
homebridge-1  | scaleAndUpdateCurrentPosition: 0
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 100
homebridge-1  | this.positionCurrent: 0
homebridge-1  | scaleAndUpdateCurrentPosition: 100
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 1
homebridge-1  | this.positionCurrent: 100
homebridge-1  | scaleAndUpdateCurrentPosition: 1
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 1
homebridge-1  | this.positionCurrent: 1
homebridge-1  | scaleAndUpdateCurrentPosition: 1
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 2
homebridge-1  | this.positionCurrent: 1
homebridge-1  | scaleAndUpdateCurrentPosition: 2
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 11
homebridge-1  | this.positionCurrent: 2
homebridge-1  | scaleAndUpdateCurrentPosition: 11
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 21
homebridge-1  | this.positionCurrent: 11
homebridge-1  | scaleAndUpdateCurrentPosition: 21
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 31
homebridge-1  | this.positionCurrent: 21
homebridge-1  | scaleAndUpdateCurrentPosition: 31
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 41
homebridge-1  | this.positionCurrent: 31
homebridge-1  | scaleAndUpdateCurrentPosition: 41
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 51
homebridge-1  | this.positionCurrent: 41
homebridge-1  | scaleAndUpdateCurrentPosition: 51
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 61
homebridge-1  | this.positionCurrent: 51
homebridge-1  | scaleAndUpdateCurrentPosition: 61
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 71
homebridge-1  | this.positionCurrent: 61
homebridge-1  | scaleAndUpdateCurrentPosition: 71
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 81
homebridge-1  | this.positionCurrent: 71
homebridge-1  | scaleAndUpdateCurrentPosition: 81
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 91
homebridge-1  | this.positionCurrent: 81
homebridge-1  | scaleAndUpdateCurrentPosition: 91
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 100
homebridge-1  | this.positionCurrent: 91
homebridge-1  | scaleAndUpdateCurrentPosition: 100
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 100
homebridge-1  | this.positionCurrent: 100
homebridge-1  | scaleAndUpdateCurrentPosition: 100
homebridge-1  | isStopped: true

homebridge-1  | updateState: 
homebridge-1  | didStop: true
homebridge-1  | latestPosition: 100
homebridge-1  | this.positionCurrent: 100
homebridge-1  | scaleAndUpdateCurrentPosition: 100
homebridge-1  | isStopped: true

@itavero It seems to me, that isStopped shouldn't always be true. Right?

Edit#2: Also, another big reason for the weird UX glitches in Home.app is that we're setting CurrentPosition to the same value as TargetPosition, right as the covering starts to move. see below 👇

Edit#3: Reverting this particular commit https://github.com/Koenkk/zigbee-herdsman-converters/commit/dfd46d6c8c7f4781ded45fc00b023660a6bee63e (i.e. not returning the set value) makes the UX noticeably better, but isStopped being off still seems to be the main issue here.

itavero commented 2 months ago

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

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:

  1. TargetPosition is always updated alongside CurrentPosition.
  2. Updating CurrentPosition during movement is fine, as long as TargetPosition is only set once at the beginning.
  3. 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.
  4. 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 commented 2 months ago

@itavero I guess we can approach this from three different directions:

  1. Implement the three suggestions from above to get a fairly good experience with most devices.
  2. Refactor and only implement the lowest common denominator (e.g. here) for a solid experience with every device.
  3. Add manufacturer/device-specific versions of cover.ts to provide a perfect experience with a select few, and a good enough experience with all other devices.
burmistrzak commented 2 months ago

@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:

  1. The set target position is not ignored. See the corresponding logs here https://github.com/itavero/homebridge-z2m/blob/c02eb2d668dd3e8a021aac529a6bc45aca14a661/src/converters/cover.ts#L180-L185
  2. 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 https://github.com/itavero/homebridge-z2m/blob/c02eb2d668dd3e8a021aac529a6bc45aca14a661/src/converters/cover.ts#L257-L263
  3. PositionState is not updated when controlled from outside of HomeKit, e.g. in-wall switch or MQTT.
burmistrzak commented 2 months ago

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

@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 https://github.com/itavero/homebridge-z2m/pull/854 with a proper TypeScript implementation later. Cool?

burmistrzak commented 1 month ago

@itavero So, https://github.com/itavero/homebridge-z2m/pull/854 is ready for review! 🤓

itavero commented 1 month ago

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

burmistrzak commented 4 days ago

Yey! Thanks for merging! 🥳