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

move_to_level_with_on_off: don't send on_off when already on #2015

Merged
merged 1 commit into from
Dec 29, 2022

Conversation

Shulyaka
Copy link
Contributor

Fixes #1554
See also: #1748, #1754

The problem:
ZCL defines the move_to_level_with_on_off which HA zha integration uses to control the brightness.
The Tuya dimmers don't have this command. The quirk has implemented this command by subsequently issuing on_off and move_to_level commands.
However it was found out that not all Tuya dimmers behave same on the on_off command: some of them switch brightness to 100% upon it, that means that changing brightness from 50% to 55% will first briefly transition through 100%.

This PR addresses the issue by remembering the current state and only issuing on_off command when needed:

  1. If the device is off and the requested brightness level is positive, the current behavior is preserved (on_off, then move_to_level)
  2. If the device is already on and the requested brightness level is positive, only move_to_level is issued
  3. If the device is on and the requested brightness level is zero, only on_off command is issued to switch off the device
  4. If the device is off and the requested brightness level is zero, the command does nothing

To save the current state, the TuyaMCUCluster was enhanced to save the respective attributes on tuya_mcu_command.
And to do that, an additional field cluster_name was added to the TuyaClusterData struct.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3721583172

  • 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 83.585%

Totals Coverage Status
Change from base Build 3697869609: 0.05%
Covered Lines: 6706
Relevant Lines: 8023

💛 - Coveralls

@codecov-commenter
Copy link

Codecov Report

Base: 83.53% // Head: 83.58% // Increases project coverage by +0.05% 🎉

Coverage data is based on head (34c8a6c) compared to base (954745e).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2015      +/-   ##
==========================================
+ Coverage   83.53%   83.58%   +0.05%     
==========================================
  Files         249      249              
  Lines        8016     8023       +7     
==========================================
+ Hits         6696     6706      +10     
+ Misses       1320     1317       -3     
Impacted Files Coverage Δ
zhaquirks/tuya/ts0601_rcbo.py 98.62% <ø> (ø)
zhaquirks/tuya/ts0601_siren.py 93.96% <ø> (ø)
zhaquirks/tuya/mcu/__init__.py 99.52% <100.00%> (+0.01%) ⬆️
zhaquirks/tuya/__init__.py 73.47% <0.00%> (+0.52%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dmulcahey
Copy link
Collaborator

We kind of handle a bit of this in HA: https://github.com/home-assistant/core/blob/df23f031314b18303d76a99f9f35e40949d3bb22/homeassistant/components/zha/light.py#L287

we have logic in there that determines which method to call. How does this differ?

@dmulcahey
Copy link
Collaborator

Also, whatever is implemented should align to the spec as close as possible. There are some differences in the newest spec for level control devices (we still haven't really updated ZHA to account for it yet.)

@Shulyaka
Copy link
Contributor Author

we have logic in there that determines which method to call. How does this differ?

The logic in ZHA solves an opposite problem - it makes sure that the on_off command is additionally issued for some devices. And this PR makes sure it is not called when not needed.
Anyway, whatever ZHA does, the quirk should correctly implement the mandatory move_to_level_with_on_off command, which it currently does not according to #1554.

Also, whatever is implemented should align to the spec as close as possible. There are some differences in the newest spec for level control devices (we still haven't really updated ZHA to account for it yet.)

I believe in this PR we do follow the specs. The only difference is that we use min level of 0 instead of 1, but this PR does not change that. I compared against Table 3-146 in Revision 8 ZCL specs.

@dmulcahey dmulcahey merged commit 5eed71e into zigpy:dev Dec 29, 2022
@CobraDunn
Copy link

On Home Assistant 2023.3.1 with a Moes double dimmer TS0601 signature _TZE200_fjjbhx9d. The lights still flash to 100% every time the dimmer reaches its target value. Is this still a known issue? Is there a quirk tweak to resolve with this device?

@TheJulianJES
Copy link
Collaborator

@CobraDunn It's easier to just create a new issue now.
Make sure to fill-out the issue template (provide device signature/diagnostics)

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.

[BUG]Unexpected interaction with New Dimmer quirk and Dimmer switch TS0601 _TZE200_1agwnems
6 participants