Remove the call to on_off cluster in TuyaLevelControl
#1748
Conversation
This PR will remove the call to `on_off` cluster when calling the `move_to_level_with_on_off` in the `TuyaLevelControl` (MCU) cluster.
|
But what will happen to the What is clear to me is that the current implementation of |
Pull Request Test Coverage Report for Build 3039989687
💛 - Coveralls |
|
Per the spec when using move_to_level_with_on_off 0 means turn the light off:
Is that not what was happening? |
|
I have no evidence, I bet you a lot of money that we have Tuya devices that don't do it and that stay bright at a minimum 🤷🏻♂️ |
|
Note that I am addressing only Tuya devices with MCUs (very far from standard-compliant) |
|
I should have looked at the code change first. I completely agree with what you have changed here. |
|
You all done w/ this? |
|
Another approach could be to replicate all the brightness related treatment from the |
|
Yes I think that is the right approach. |
The previous version was 🤦🏻♂️ |
|
I dont remember but i think 0 in light level is not the same the the light is off 0x0006 can still being on. Light level is little tricky and can have status then light is off and more strange things. From |
See this comment: #1748 (comment) |
|
Later I review the open issues for this reason and report the change. Thanks to both. |
|
|
no, this is straight from zigbee cluster library specification. Also I am fairly certain that it's anything less than the minimum and not just === the minimum. |
|
this is an older one and it is section 3.10.2.4.5 but it is consistent with the newest version from above. |
|
OK from ZCL R8: |
|
you are looking at attribute docs and not command docs. here is another from the same section in the level control cluster:
specifically: "When the level is reduced to its minimum the OnOff attribute is automatically turned to Off, and when the level is increased above its minimum the OnOff attribute is automatically turned to On." I think this confirms my thoughts above that they don't have to match exactly and that simply surpassing the value is what triggers the change. |
|
That is true minimum = 1 if reading both then you have not red ZERO. |
|
min level is documented as accepting 0 |
|
this is not ZCL or the ZHA profile specification this is in ZLO. |
|
Nop its the tales ZCL R8 |
|
it's a different cluster lol. |
|
7554 3.19.1.3 Cluster Identifiers |
|
I think all the logic is explained in: |
|
ok so now I am honestly confused. this overrides CurrentLevel definition of the base cluster but nothing else... say we follow this... all that changes is that in HA (ZHA) we would cap 0 to 1. The experienced functionality would not change. Are you saying if we in fact send 1 that the bulb would behave correctly? |
|
Read the posted min level on level cluster is allowed to have and and then the last posted Device min level is >1 or if sett in the device (but cant being less then 1) and the matrix is the logic that is being used if its one OK ZB3 device. I only saying dont use Zero then its not allowed in ZB3 devices that is not to old. If tuya is having 53629947 as min its in the end ZCL R7.5 that is being used in ZHA = larger then zero. |
|
I wonder... I bet this is related to your comments @MattWestb home-assistant/core#78206 These would all be changes for HA / ZHA and I think we are on the path to getting to an agreement in there... doesn't affect this though imo |
|
I don't think this PR solves the underlying issue. Instead of sending the command to the zigbee device, the quirk handles the command itself and directly controls the attributes of the device. Therefore, setting the on_off attribute when the move_to_level_with_on_off command is called is correct according to the ZCL spec. Otherwise, you're only ever setting the current_level attribute, which is independent of the on_off attribute, so the devices using this quirk will never turn on or off when the move_to_level_with_on_off command is sent. The current implementation in the master branch is wrong, since it assumes args[1] is a boolean "on_off" parameter. But the solution should be to use args[0], the level, and set on_off according to that. With that implementation, the device would "correctly" handle the command (via its quirk), so ZHA would not need to add extra handlers. |
As I've already said, ZHA offers a mechanism to work around that problem:
I am not convinced on this point. It might seem like a simple solution, but we would duplicate the code in 2 places (ZHA and in the quirk). It may seem that it is a minor problem, but it is not, and you have the example in the conversation that @dmulcahey and @MattWestb have had. There is a flaw in the handling (and interpretation) of the dimmers But... the main drawback that I see in this approach is that the solution involves adding the code of the @STRICT_MATCH(
channel_names=CHANNEL_ON_OFF,
aux_channels={CHANNEL_COLOR, CHANNEL_LEVEL},
manufacturers={"Jasco", "Quotra-Vision", "eWeLight", "eWeLink"},
)
class ForceOnLight(Light):
"""Representation of a light which does not respect move_to_level_with_on_off."""
_FORCE_ON = TrueIn the Tuya devices this is the And this is the dilemma that must be solved... |
|
I understand your concerns, code duplication is never good. Since I'm fairly new to zigpy/zha-device-handlers/HA, I might be also be overlooking some aspects of the architecture that would break or add unnecessary overhead with my proposed approach. Nevertheless, I still believe that the correct place to fix this is in the quirk, and not in ZHA.
While it is a simple solution, I wasn't proposing it due to its simplicity. The description of the zha-device-handlers project states that its function is to handle deviations from the ZCL standard in order to provide HA with a standard-compliant interface. In this case, the quirk exposes the move_to_level_with_on_off command, but it's not following the standard itself, since it's not doing anything about the _with_on_off part. If you work around that bug by changing HA, you're effectively adding another "quirks" layer to handle the issues in the zha-device-handlers code. I think the "quirks" functionality (i.e. code that works around ZCL spec violations) should be kept in a single layer, because every layer that works around issues of the one below is an additional component that you have to keep synchronized and will likely break if someone decides to tweak or fix the incorrect behavior.
I had that feeling, since my HA dimmer sliders will happily go down to 1% even though the dimmers do not go darker than the value set at around 20%. Unfortunately, I would not know where to start to fix this. At least one layer would have to fetch the minLevel and maxLevel from the zigbee device and cache those values in order to adapt what 1% and 100% mean. |
|
@dmulcahey and I discussed yesterday what @MattWestb pointed about about the spec and I think the current behavior in ZHA (and the entire From what I gather:
I'm not sure if our understanding of the spec is correct, but it seems to match up with what that eWeLight bulb is doing and how existing bulbs on our networks handle |
|
The implanted tuya MCU min and max dimmer is only reading and writing values and mapping them after conversation to ZHA Zigbee standard. If not making it 100% true is not one large problem if ZHA is tolerant for the 0 light level attribute (but shall not being that if being ZCL 7.5 compatible). But as i have saying better doing it right the first time or you is getting larger problems later (Z2M is using 256 as max and its 110% wrong). With the "device min level" can being larger then 1 if the device have getting the attribute set to somthing else then standard so its making its more complicated if the device is having 25 sett and then need taking that in the calculation. Great having our P online then doing ZCL talking !!! Also ZLO was integrated in one of the latest ZCL reversions and i think ZHA have not being updated with that part to 100%. |
|
Too many paralel conversations here... @gonzalezjj I must say that the ZHA approach I like it less and less. I would try to restore the previous behavior (this time well implemented) I would leave the implementation of Tuya devices (Tuya MCU) for last. Let's see where this road takes us and then we'll worry about them. All the |
This is true but the way we understand what is in the spec is that 1 is a special value that means use the devices minimum value. Is that not how you are reading it? #1748 (comment) “ A value of 1 SHALL indicate the minimum level that can be attained on a device.” |
Implement the `on_off` call in the `move_to_level_with_on_off` command for the MCU Tuya devices. The previous implementation was removed in zigpy#1748. Originally introduced in zigpy#1489
This PR will remove the call to
on_offcluster when calling themove_to_level_with_on_offin theTuyaLevelControl(MCU) cluster.The current quirk is completely wrong and needs to be fixed.
TL;DR;
But let me explain it from the begining...
In #1489 (ignoring some good practices) a call to the
on_offcluster is added in themove_to_level_with_on_offcommand. This change is caused by a change in HA where dimmers now invoke this command (before the call from HA was different).Some users report that the behavior of their devices has changed and that now its devices do not turn on when using the dimmer bar from HA.
The programmer in charge of #1489, misreading the arguments to the
move_to_level_with_on_offcommand, (mistakenly) setsargs[1]to theon/offvalue of the device.He thought that invoking the
on/offcommand for those devices that respected the standard couldn't hurt, and that on those that didn't it would be an improvement.What could go wrong? Spoiler:
args[1]is actually thetransition_time:This is the origin of the reported issues (or at least some of them).
Since the time this 'improvement' was implemented, ZHA has also evolved and now implements a mechanism to force the invocation of the
oncommand for those devices that do not respect the standard:With the new approach, the call is removed from the quirk because ZHA already have a mechanism to force the call to the
oncommand.Now the devices detected as problematic can be added in ZHA (just a single place).