zigpy / zha

Zigbee Home Automation
Apache License 2.0
21 stars 19 forks source link

Fix IKEA Starkvind attribute reports #125

Closed TheJulianJES closed 2 months ago

TheJulianJES commented 2 months ago

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 https://github.com/zigpy/zha/pull/87/ in the future.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dmulcahey commented 2 months ago

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