Refactor Aqara Roller Driver E1 as v2 quirk to expose configuration and status entities

[jeverley]

Proposed change

This PR refactors the existing Aqara Roller Driver E1 quirk into the new v2 structure to expose the configuration and status attributes, below is a summary of changes:

• Refactor into v2 quirk format
  • Refactor clusters to use AttributeDefs format
• Expose attributes as entities:
  • Motor speed (select)
  • Charging status (binary sensor)
  • Calibration status (binary sensor)
• Represent the device as a Rollershade WindowCoveringType rather than Drapery
• Device cluster logic
  • Reads/reports of AnalogOutput: present_value (current position) are replicated into WindowCovering: current_position_lift_percentage
  • Writes to AnalogOutput: present_value (go to position) do not update the WindowCovering: current_position_lift_percentage
  • Reads of WindowCovering: current_position_lift_percentage are redirected to AnalogOutput: present_value
  • Window Covering cluster commands:
    • up_open, down_close, stop are handled by MultistateOutput: present_value
      • stop additionally issues a read to AnalogOutput: present_value
    • go_to_lift_percentage value is written to AnalogOutput: present_value

Functional enhancements

• Users can change the Motor speed using the Select entity
• Users can see if the device is Charging
• Users can see if the device is Calibrated
• The cover entity is a Rollershade rather than Drapery (Curtains)
• The redundant number entity for current position is disabled by default

Additional information

Depends on the following PRs for binary_sensor value conversion:

• Add attribute_converter to quirks v2 sensors zigpy#1540
• Implement quirks v2 attribute_converter zha#360

Depends on the following PRs for disabling the redundant number entity:

• Allow v2 quirks to opt out of default entity registration zigpy#1535
  • [RFC] Explicit entity object discovery, lifetimes, and registration zha#365 required for this to do anything in ZHA.

Whilst I've validated that is works with my own E1 roller shade devices, I'd like to ensure other users do not encounter any regressions with this refactor.
@dmulcahey / @schwickster / @javicalle / @badrpc / @TheJulianJES I can see in the past you've previously contributed for this device, if you still use it would you mind checking if this updated quirk works as expected without introducing regressions?

Checklist

• The changes are tested and work correctly
• pre-commit checks pass / the code has been formatted using Black
• Tests have been added to verify that the new code works

[codecov]

Codecov Report

Attention: Patch coverage is 98.71795% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.95%. Comparing base (8b3560e) to head (ad03e30).

Files with missing lines	Patch %	Lines
zhaquirks/xiaomi/aqara/roller_curtain_e1.py	98.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #3686      +/-   ##
==========================================
+ Coverage   90.91%   90.95%   +0.03%     
==========================================
  Files         325      325              
  Lines       10566    10623      +57     
==========================================
+ Hits         9606     9662      +56     
- Misses        960      961       +1     

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

[jeverley]

Have identified an issue with the cover state not reliably changing to open/closed. Investigating this currently.

[jeverley]

@TheJulianJES, @dmulcahey looking at the code in https://github.com/zigpy/zha/blob/dev/zha/application/platforms/cover/__init__.py
it looks to me like the _determine_state function and stored target positions might be the cause of the issue, I'm seeing similar issues reported for other devices which would appear to have the same root cause as below:

• ewand cover not reporting open/close state correctly through ZHA home-assistant/core#93836
• ZHA : Cover states stays as Opening or Closing when using Set position home-assistant/core#98933
• [Device Support Request] Insta GmbH Nexentro Blinds Actuator Mini #1397

There was a PR against the old core ZHA repo to try and address this but that looks abandoned home-assistant/core#105467

I've tested the following scenarios:

1. go_to_lift_percentage has never been called, down_close is called, then the device reports position_or_tilt of 0
_target_lift_position == None
position_or_tilt == 0
returns STATE_CLOSED

2. go_to_lift_percentage has never been called, up_open is called, then the device reports position_or_tilt of 100
_target_lift_position == None
position_or_tilt == 100
returns: STATE_OPEN

3. go_to_lift_percentage is called (go to 0), then the device reports position_or_tilt of 0
_target_lift_position == 0
position_or_tilt == 0
returns STATE_CLOSED

4. go_to_lift_percentage is called (go to 100), then the device reports position_or_tilt of 100
_target_lift_position == 100
position_or_tilt == 100
returns STATE_OPEN

5. go_to_lift_percentage is called (go to 50), then the device reports position_or_tilt of 50
_target_lift_position == 50
position_or_tilt == 50
returns STATE_OPEN

6. go_to_lift_percentage has previously been called (stored value is 50), down_close is called, then the device reports position_or_tilt of 0
_target_lift_position == 50
position_or_tilt == 0
returns STATE_CLOSED

7. go_to_lift_percentage has previously been called (stored value is 50), up_open is called, then the device reports position_or_tilt of 100
_target_lift_position == 50
position_or_tilt == 100
returns None, state remains as STATE_OPENING

8. go_to_lift_percentage has previously been called (stored value is 50) and current position is 100, stop is called
_target_lift_position == 100 (the function async_stop_cover sets it to the current cover position)
position_or_tilt == 100
returns STATE_OPEN

I'd suggest we can handle this in one of 3 ways:

1. set the stored _target_lift_position to 100 or 0 when calling the up_open or down_close commands respectively
2. clear the stored _target_lift_position when calling the up_open or down_close commands
3. modify the _determine_state function to return STATE_OPEN when self.is_opening == True and position_or_tilt == 100 is true

Below is the current function for reference:

    def _determine_state(self, position_or_tilt, is_lift_update=True) -> None:
        """Determine the state of the cover.

        In HA None is unknown, 0 is closed, 100 is fully open.
        In ZCL 0 is fully open, 100 is fully closed.
        Keep in mind the values have already been flipped to match HA
        in the WindowCovering cluster handler
        """
        if is_lift_update:
            target = self._target_lift_position
            current = self.current_cover_position
        else:
            target = self._target_tilt_position
            current = self.current_cover_tilt_position

        if position_or_tilt == 0:
            self._state = (
                STATE_CLOSED
                if is_lift_update
                else STATE_OPEN
                if self.current_cover_position is not None
                and self.current_cover_position > 0
                else STATE_CLOSED
            )
            return
        if target is not None and target != current:
            # we are mid transition and shouldn't update the state
            return
        self._state = STATE_OPEN

[jeverley]

I've been working on changes to the ZHA cover entity hander to address the underlying status issue outlined above (it also affects the existing v1 quirk code for this and other devices).

I'll raise a bug and PR to address these over the weekend, current WIP code here:
https://github.com/jeverley/zha/blob/cover-platform-entity-status/zha/application/platforms/cover/__init__.py

[jeverley]

@TheJulianJES if I'm reading zigpy/zigpy#1540 I believe I should be able to use the proposed attribute_converter functionality to remap the Aqara values for the charging binary_sensor?

If so I'll remove the custom converter I've implemented in this quirk once your PR is merged.

[jeverley]

@TheJulianJES the AnalogOutput cluster present_value on this device is used to write the intended target position, whereas reads/reports on the attribute report the device current position.

Right now ZHA exposes this as a separate number entity in addition to the cover entity.
In the UI the number entity is redundant since interactions & current position can all be handled through the cover.

When adding custom entities in a v2 quirk we're able to mark them as disabled by default, do you think it would be beneficial to extend this to regular cluster attributes?
This would allow us to present devices more uniformly, without exposing redundant entities arising from manufacturers' non-standard implementations.

Critically this would be different to entirely removing the cluster/marking an attribute as unsupported in the cluster, since we'd still want to remap attribute updates/writes from the cover commands to the cluster.

[TheJulianJES]

When adding custom entities in a v2 quirk we're able to mark them as disabled by default, do you think it would be beneficial to extend this to regular cluster attributes?

With zigpy/zigpy#1535, that should be allowed in the future. Do note that the ZHA part wasn't PR'd yet, so it won't do anything for now.
attribute_converter support also isn't merged, yet.

[jeverley]

@TheJulianJES I think this should be ready for merging, I'll raise a PR for the zha cover status issue outlined in #3686 (comment) separately (it's not a pre-req for this change).

[puddly]

For this use case, why remove the cluster entirely? I'm hoping to replace uses of removes with prevent_default_entity_creation.

[jeverley]

As far as I can tell this cluster is non-functional.

Interacting with the switch results in an error, so there would be no benefit exposing it.
If it's an optional entity a user might be tempted to enable it and raise a bug report that the switch doesn't work.

[puddly]

I think removes(OnOff.cluster_id) can be replaced with prevent_default_entity_creation(cluster_id=OnOff.cluster_id). Removing the cluster prevent prevents automatic entity discovery, which is more explicit with prevent_default_entity_creation and would allow you to still interact with the OnOff cluster via cluster commands if necessary.

[jeverley]

I'd usually agree, but the device itself is responding with the below error to any command to OnOff:

Failed to perform the action zha/issue_zigbee_cluster_command. Failed to issue cluster command with status: <Status.UNSUPPORTED_CLUSTER: 195>