Skip to content

Fix current_cover_tilt_position for shades, add base class #416

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 14, 2025

Conversation

TheJulianJES
Copy link
Contributor

@TheJulianJES TheJulianJES commented Apr 12, 2025

Proposed change

This PR fixes an issue where the current_cover_tilt_position property was removed for the Shade implementation, based on the LevelControl cluster (in #376).

The Home Assistant integration does not differentiate between our Cover and Shade implementation, so tries to access the current_cover_tilt_position attribute/property regardless: https://github.com/home-assistant/core/blob/d23c9f715e1f7772ec8ae176f3db4b9afa7db95d/homeassistant/components/zha/cover.py#L122-L125

We need to make sure we provide it in all our cover implementations, so this PR adds it back, returning None by default, as a Shade cannot support tilt.

A new abstract BaseCover class is also added to make sure all relevant methods are implemented by the Cover and Shade implementations.

Additional information

Should address:

We should likely add tests for this in HA in the future. I'm guessing Shade is not covered at all.

@TheJulianJES TheJulianJES added the bugfix This PR fixes a bug label Apr 12, 2025
Copy link

codecov bot commented Apr 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.69%. Comparing base (2e8fbe8) to head (2195961).
Report is 1 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #416   +/-   ##
=======================================
  Coverage   96.68%   96.69%           
=======================================
  Files          61       61           
  Lines        9834     9863   +29     
=======================================
+ Hits         9508     9537   +29     
  Misses        326      326           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@puddly
Copy link
Contributor

puddly commented Apr 12, 2025

Could we derive both of them from a single abstract base class that implements the proper API?

@TheJulianJES
Copy link
Contributor Author

TheJulianJES commented Apr 12, 2025

The added line is uncovered, but it looks like all ZHA tests generally use the state property to access various state of the entities. But we do not include the tilt percentage in the state for shades, as it's always None, so we'll need to access that property directly in tests, should be fine though.
-> Added in 2195961 (#416)

Compared to accessing the state via state in ours tests, HA directly accesses the different properties.

@TheJulianJES
Copy link
Contributor Author

TheJulianJES commented Apr 12, 2025

Could we derive both of them from a single abstract base class that implements the proper API?

Probably.

@TheJulianJES
Copy link
Contributor Author

TheJulianJES commented Apr 12, 2025

The implementations don't share a lot, so the commit is really only the shared API used by HA and setting PLATFORM + _attr_primary_weight.
Also, it seems like ruff doesn't enforce whether to have pass or just the docstring in empty methods. What do we prefer for that? (edit: I guess nothing, so Codecov doesn't complain)

async_open_cover_tilt, async_close_cover_tilt, async_set_cover_tilt_position, and async_stop_cover_tilt are methods only used by Cover from HA. I've not added these to the base class, as we'd also need to implement them for Shade otherwise, and HA should only call them if the supported_features contain the corresponding tilt features.

(On an unrelated note, we should not really set _attr_translation_key if a device class is already present, as the generic device class names should be fine for our purpose and we don't really need another one yet.)

@TheJulianJES TheJulianJES changed the title Fix current_cover_tilt_position missing for shades Fix current_cover_tilt_position for shades, add base class Apr 13, 2025
@puddly puddly merged commit a1897cb into zigpy:dev Apr 14, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants