Fix Sinope device triggers

[ckm2k1]

Proposed change

Fixes device automation triggers for the Sinope light quirk. The current definition of the automation triggers
uses the ButtonAction enum in the value field, which throws an error from Voluptuous when passed into HA's async_attach_trigger.

Additional information

Not a breaking change and doesn't require anything else to be merged into HA Core.

Checklist

• The changes are tested and work correctly
• pre-commit checks pass / the code has been formatted using Black
• Tests have been added to verify that the new code works

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.44%. Comparing base (728ee42) to head (3bae0da).

Additional details and impacted files

@@           Coverage Diff           @@
##              dev    #3504   +/-   ##
=======================================
  Coverage   89.44%   89.44%           
=======================================
  Files         311      311           
  Lines       10033    10033           
=======================================
  Hits         8974     8974           
  Misses       1059     1059           

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

[ckm2k1]

I noticed an issue while trying to debug this problem.

To validate my fix, I added a modified copy of the sinope quirk into the custom quirks directory in my live HA instance.
Although ZHA loads the modified quirk just fine, since the fix itself happens at definition time, my modifications are loaded too late in the process to fix the call to async_attach_trigger in /zha/core/homeassistant/components/zha/device_trigger.py.

Instead, I had to ssh into HA OS as root and modify the zhaquirks code manually in /usr/local/lib/python3.12/site-packages/zhaquirks/sinope/__init__.py. This worked, but is pretty tedious if you're trying to iterate and check your changes.

I don't know if it's possible, but it would be nice if the loading order was changed such that if a custom quirk is present and it overrides an existing quirk, it would be loaded instead of the builtin one, and not after it.

---

Separately, I also noticed that there's a helper schema_type_to_vol, which is called on cluster command arguments, likely to prevent the exact problem I'm trying to fix in this PR.

If all fields in device automation triggers were either validated at dev time, or better yet, automatically coerced into voluptuous supported types, quirk authors wouldn't be forced to use scalar types instead of Enum values.

[ckm2k1]

@bassdr a couple questions for you:

• Does the above comment sound like what you ran into here: Send all button press events for Sinope SW2500ZB/DM2500ZB #3438 (comment)? If so, how did you work around it?
• Did all your button related automations also break with HA 2024.11.0?

[bassdr]

@ckm2k1
#3504 (comment)
I don't know if it's possible, but it would be nice if the loading order was changed such that if a custom quirk is present and it overrides an existing quirk, it would be loaded instead of the builtin one, and not after it.

@bassdr a couple questions for you:

• Does the above comment sound like what you ran into here: Send all button press events for Sinope SW2500ZB/DM2500ZB #3438 (comment)? If so, how did you work around it?

Yes, this problem is exactly why I used relative import paths. This way, you can add the __init__.py file to your quirk and it works just fine. See f922d14, actually, you want to revert this one.
So in short, in your config folder create a folder called zhaquirks, inside you create a folder named sinope, and you setup your configuration.yaml to point to the zharquirks folder (not the sinope folder). The sinope folder must include the __init__.py and all files that will be affected by the changes (take the whole folder just to be safe). You then have to change everywhere you from zhaquirks.sinope import to from . import. It just works. I did not understand the point of having absolute paths but, not the end of the world either...

• Did all your button related automations also break with HA 2024.11.0?

I haven't tried 2024.11.0 yet, I keep my non-dev HA setup on the last version of last month... But I'm surprised to see this PR indeed, I'll upgrade to check when I get the chance. Do you still have the exact voluptuous error it gave you? Or maybe a snippet of you automation that is giving the error? I'm personally listening to the zha event directly like this:

  - trigger: event
    event_type: zha_event
    event_data:
      endpoint_id: 1
      cluster_id: 65281
      args:
        attribute_name: action_report

To test it's the right device:

condition: template
value_template: |-
  {{
    device_id('light.upstairslightswitch_lumiere') == trigger.event.data.device_id
  }}

and eventually:

choose:
  - conditions:
      - condition: template
        value_template: "{{ trigger.event.data.command == 'initial_press' }}"
    sequence:
      - action: zha.issue_zigbee_cluster_command
        data:
          ieee: |-
            {{
              device_attr('light.upstairslightswitch_lumiere', 'identifiers')
                | selectattr(0,'eq','zha')
                | map(attribute=1)
                | first
            }}
          endpoint_id: 1
          cluster_id: 8
          cluster_type: in
          command: 5
          command_type: server
          params:
            move_mode: |-
              {{
                iif (trigger.event.data.args.button == 'turn_on', 'Up', 'Down')
              }}
            rate: 50
    alias: Initial Press

[bassdr]

I can't find the schema of device_automation_triggers, TBH that might be the part I missed in the original PR, but I assumed this was not fixed anywhere.

EDIT: I think I figured my original mistake: the device_automation_triggers should either listen to description field or be casted to int in the value field like you are doing in this PR, as emitted here:

        event_args = {
            ATTRIBUTE_ID: 84,
            ATTRIBUTE_NAME: ATTRIBUTE_ACTION,
            BUTTON: button,
            DESCRIPTION: action.name,
            VALUE: action.value,
        }

action.value here is equivalent to int(action)

[bassdr]

Good catch, sorry about it.

I personally prefer VALUE: ButtonAction.Pressed_on.value to match the syntax when we emit.

Another fix would be to change to DESCRIPTION: ButtonAction.Pressed_on.name, but I don't have a strong preference.

[ckm2k1]

So do I :)

Problem is that when I tested using .value, it turned out that it's of type <class 'zigpy.types.basic.uint8_t'> and not an int! This still made Voluptuous raise an invalid type exception, so I opted for the next simplest solution, coercing to int.

[bassdr]

I'm OK with this test, but like I said in the other comment, I don't think it's forced anywhere that the value field is an int. I personally think zha should fix rules like this tho (so we don't have to do it in the sinope test, but all fields would be tested at once)

[ckm2k1]

I agree in principal, and would happily participate in a discussion with the ZHA maintainers, but since there are no guarantees, I'd prefer to patch things in a simple way and have our automations work like before.

[ckm2k1]

I don't think it's forced anywhere that the value field is an int

This is true, but it's indirectly forced by the fact that Voluptuous fails to handle user types. I did find the root of the problem, described in my comment here: #3504 (comment)

[ckm2k1]

@bassdr

I haven't tried 2024.11.0 yet, I keep my non-dev HA setup on the last version of last month... But I'm surprised to see this PR indeed, I'll upgrade to check when I get the chance. Do you still have the exact voluptuous error it gave you? Or maybe a snippet of you automation that is giving the error? I'm personally listening to the zha event directly like this:

  - trigger: event
    event_type: zha_event
    event_data:
      endpoint_id: 1
      cluster_id: 65281
      args:
        attribute_name: action_report

...

I'm not sure the above is needed anymore after these PRs: #3313 and zigpy/zha#153.

I got tired of writing YAML for these things (I have a whole bunch of Sinope devices, particularly light switches), so I submitted the PRs above to patch things and make it work through the UI. Here's a partial example of an automation I'm running: