Skip to content
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

Fix IKEA Starkvind attribute reports #125

Merged
merged 2 commits into from
Aug 4, 2024

Conversation

TheJulianJES
Copy link
Contributor

@TheJulianJES TheJulianJES commented Aug 4, 2024

Proposed change

When the fan_mode attribute was updated, the attribute_updated method was called again. Repeat this.
Eventually, we would reach the maximum recursion depth and error out.

Now, we're completely removing the overridden attribute_updated method in the IKEA manufacturer-specific cluster.
Instead, the super method is now called which does what we want (emit an event), but for all attributes. This is needed, as the child_lock and other config attributes on that cluster would not cause the config switch entities to update if changed from the device.

The change is tested and seems to be working as expected now.

Additional information

On a side note, the attribute_updated stuff in the other cluster handlers also needs be improved. There's some duplicate code that can be avoided and it's one of the last places where the old _value_attribute stuff is still used.
It's also a bit confusing if only one attribute change actually emits an "attribute updated" event on a cluster. Maybe we should always emit the events and just check for the attribute in the places where we're subscribing? (the config switches do this already) It shouldn't cause any performance issues really..

Also, it seems like we do have a unit test for this in test_fan.py, but it didn't catch the issue, as we're (1) not testing getting a fan_mode attribute report from the device and/or (2) because when fan_mode is changed from ZHA, we're only checking if the attribute was written correctly. Not if the entity was also updated correctly.
Doing either of those things would have caught the issue.

Further fixes and changes will come with #87 in the future.

Copy link

codecov bot commented Aug 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.68%. Comparing base (248faf3) to head (2ebb1c0).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #125      +/-   ##
==========================================
- Coverage   95.69%   95.68%   -0.01%     
==========================================
  Files          61       61              
  Lines        9260     9255       -5     
==========================================
- Hits         8861     8856       -5     
  Misses        399      399              

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

@dmulcahey
Copy link
Contributor

@TheJulianJES can you open issues for the things mentioned in “additional information” please.

@dmulcahey dmulcahey merged commit 4258c27 into zigpy:dev Aug 4, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants