Add support for QUAD-ZIG-SW from smarthjemmet.dk #3400

mortenmoulder · 2024-10-06T19:35:13Z

Proposed change

Add support for smarthjemmet.dk's QUAD-ZIG-SW device.

Additional information

Works with V1 and V2 firmware.

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

…dlers into dev

codecov · 2024-10-06T20:21:59Z

Codecov Report

Attention: Patch coverage is 71.42857% with 6 lines in your changes missing coverage. Please review.

Project coverage is 89.35%. Comparing base (7816838) to head (544880b).
Report is 27 commits behind head on dev.

Files with missing lines	Patch %	Lines
zhaquirks/smarthjemmet/quadzigsw.py	71.42%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #3400      +/-   ##
==========================================
+ Coverage   88.72%   89.35%   +0.62%     
==========================================
  Files         306      310       +4     
  Lines        9813    10027     +214     
==========================================
+ Hits         8707     8960     +253     
+ Misses       1106     1067      -39

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

…dlers into dev

mortenmoulder · 2024-10-06T20:24:25Z

All should be fixed now. Sorry for all the commits.

mortenmoulder · 2024-10-17T22:30:56Z

@TheJulianJES I see that a new release was created yesterday, and mine was not included. Any idea why? The only checks failing are code coverage, which shouldn't prevent it from being merged. Was really hoping to get this quirk implemented in the next release, as I'm guessing I have to wait a few more weeks now.

TheJulianJES · 2024-10-17T23:08:01Z

There's a few things on this PR that I'd likely change. I just haven't found the time to properly review this PR and comment all small change suggestions. I still plan to do that, of course, but there are also other PRs.
Yesterday's release was just to get some bigger ZHA related changes into nightly HA builds. The quirks related changes were mostly ones that didn't need feedback.

Follows the style of other quirks where there's proper quirk defined in a `CustomDevice` class that's inherited from in other quirks that just change the `signature`, but keep the `replacement` and device triggers.

TheJulianJES · 2024-10-18T00:19:46Z

Ok, so I've pushed multiple commits now. The "refactor" commit changes the structure of the (so-called v1) quirk to have a v1 device class with signature, replacement, and device triggers.
Then, a class for firmware version 2 is added that inherits from the first device class. It just changes the signature.
This is similar to the structure we have in other quirks.

The last commit (as of now) "Replace with v2 quirk" completely replaces the CustomDevice v1 quirks with a v2 quirk.
For it, we don't need to have a fixed signature and replacement. .replaces(CR2032PowerConfigurationCluster) will automatically replace all clusters with the same cluster id on endpoint 1. It's used to replace the default PowerConfiguration cluster with the custom CR2032PowerConfigurationCluster.

.replace_cluster_occurrences(CustomMultistateInputCluster) will look at all endpoints and replace all clusters with the same cluster id (of MultistateInput) it can find.

Please test if that v2 quirk works as expected.

With the v1 quirk before, the MultistateInput cluster was always an output/client cluster in the original signature. In the replacement, it was a input/server cluster then. It was also missing on endpoint 1 in the replacement. I don't think that was intentional?

If the replacement CustomMultistateInputCluster needs to be an input/server cluster (so the role swapped compared the original signature), we can do it like this in a v2 quirk (but for all endpoints):

    .removes(MultistateInput.cluster_id, cluster_type=ClusterType.Client, endpoint_id=1)  # remove output cluster
    .adds(CustomMultistateInputCluster, endpoint_id=1)  # default cluster_type is server, so it'll add an input cluster

The test here is no longer really needed for the v2 quirk. There's no real point in testing if all the correct clusters are included. Ideally, we would send messages from the device and see if they are converted to "ZHA events" as expected in a test.

TheJulianJES · 2024-10-18T00:26:18Z

zhaquirks/smarthjemmet/quadzigsw.py

+    async def bind(self):
+        """Prevent bind."""
+        return zigpy.zcl.foundation.Status.SUCCESS
+
+    async def unbind(self):
+        """Prevent unbind."""
+        return zigpy.zcl.foundation.Status.SUCCESS
+
+    async def _configure_reporting(self, *args, **kwargs):
+        """Prevent remote configure reporting."""
+        return (
+            zigpy.zcl.foundation.ConfigureReportingResponse.deserialize(b"\x00")[0],
+        )


Unrelated to converting the whole quirk to a v2 quirk:
Are these necessary? We already set SKIP_CONFIGURATION = True.

mortenmoulder · 2024-10-18T06:16:11Z

@TheJulianJES Hi and thanks for your effort. Unfortunately it's not working.

When I am talking about V1 and V2 in terms of firmware, it's because I have one physical device with two different firmware versions. The signature of the devices are completely different, which is why I need to be able to support both signatures on the same device. V1 of the firmware came out back in 2023, and now I've updated it in late 2024. Does that make sense?

For all I care, Quirk V1 or Quirk V2 doesn't matter to me. Quirk V2 looks better with the QuirkBuilder - definitely, but unfortunately it's not working as intended in this case.

Regards to your "It was also missing on endpoint 1 in the replacement. I don't think that was intentional" it actually was. The physical device has 5 buttons, but only 4 of them should work as MultistateInput. The last is used for resetting the device, which is done on a firmware level.

mortenmoulder · 2024-10-18T06:17:53Z

And for good measure, here's the old firmware (firmware V1 from 2023)'s device signature:

{
  "node_descriptor": {
    "logical_type": 2,
    "complex_descriptor_available": 0,
    "user_descriptor_available": 0,
    "reserved": 0,
    "aps_flags": 0,
    "frequency_band": 8,
    "mac_capability_flags": 128,
    "manufacturer_code": 4447,
    "maximum_buffer_size": 80,
    "maximum_incoming_transfer_size": 160,
    "server_mask": 0,
    "maximum_outgoing_transfer_size": 160,
    "descriptor_capability_field": 0
  },
  "endpoints": {
    "1": {
      "profile_id": "0x0104",
      "device_type": "0xfffe",
      "input_clusters": [
        "0x0000",
        "0x0001",
        "0x0007"
      ],
      "output_clusters": [
        "0x0000",
        "0x0001",
        "0x0012"
      ]
    },
    "2": {
      "profile_id": "0x0104",
      "device_type": "0xfffe",
      "input_clusters": [
        "0x0007"
      ],
      "output_clusters": [
        "0x0012"
      ]
    },
    "3": {
      "profile_id": "0x0104",
      "device_type": "0xfffe",
      "input_clusters": [
        "0x0007"
      ],
      "output_clusters": [
        "0x0012"
      ]
    },
    "4": {
      "profile_id": "0x0104",
      "device_type": "0xfffe",
      "input_clusters": [
        "0x0007"
      ],
      "output_clusters": [
        "0x0012"
      ]
    },
    "5": {
      "profile_id": "0x0104",
      "device_type": "0xfffe",
      "input_clusters": [
        "0x0007"
      ],
      "output_clusters": [
        "0x0012"
      ]
    }
  },
  "manufacturer": "smarthjemmet.dk",
  "model": "QUAD-ZIG-SW",
  "class": "zigpy.device.Device"
}

and here is the new firmware V2 (from late 2024) device signature:

{
  "node_descriptor": {
    "logical_type": 2,
    "complex_descriptor_available": 0,
    "user_descriptor_available": 0,
    "reserved": 0,
    "aps_flags": 0,
    "frequency_band": 8,
    "mac_capability_flags": 128,
    "manufacturer_code": 4447,
    "maximum_buffer_size": 80,
    "maximum_incoming_transfer_size": 160,
    "server_mask": 0,
    "maximum_outgoing_transfer_size": 160,
    "descriptor_capability_field": 0
  },
  "endpoints": {
    "1": {
      "profile_id": "0x0104",
      "device_type": "0x0006",
      "input_clusters": [
        "0x0000",
        "0x0001"
      ],
      "output_clusters": [
        "0x0000"
      ]
    },
    "2": {
      "profile_id": "0x0104",
      "device_type": "0x0006",
      "input_clusters": [
        "0x0007",
        "0x0012"
      ],
      "output_clusters": [
        "0x0012"
      ]
    },
    "3": {
      "profile_id": "0x0104",
      "device_type": "0x0006",
      "input_clusters": [
        "0x0007",
        "0x0012"
      ],
      "output_clusters": [
        "0x0012"
      ]
    },
    "4": {
      "profile_id": "0x0104",
      "device_type": "0x0006",
      "input_clusters": [
        "0x0007",
        "0x0012"
      ],
      "output_clusters": [
        "0x0012"
      ]
    },
    "5": {
      "profile_id": "0x0104",
      "device_type": "0x0006",
      "input_clusters": [
        "0x0007",
        "0x0012"
      ],
      "output_clusters": [
        "0x0012"
      ]
    }
  },
  "manufacturer": "smarthjemmet.dk",
  "model": "QUAD-ZIG-SW",
  "class": "QUAD-ZIG-SW.QUAD_ZIG_SW"
}

mortenmoulder · 2024-10-18T20:43:48Z

@TheJulianJES Help is not really wanted. What I originally had worked perfectly. Only thing not passing is the code coverage. Is that a requirement to get it merged?

TheJulianJES · 2024-10-18T20:45:45Z

"Help wanted" just means this needs to be looked again. A v2 quirk should work.
And yes, the code should be covered fully. That's not really a big issue though.

mortenmoulder · 2024-10-18T20:50:53Z

Okay I see. Is it for you to look at "at some point" as a reminder? Or someone else? My question really is, do I need to take action?

TheJulianJES · 2024-10-19T00:31:30Z

I'll take another look again at this soon. In the mean time, you can try to see if you can make the v2 quirk work if you want.
E.g. by copying/pasting the lines at the bottom from this comment into the quirk (replacing the replace_cluster_occurrences).
(Adding those two lines for endoints (1?) 2, 3, 4, 5.)

Regards to your "It was also missing on endpoint 1 in the replacement. I don't think that was intentional" it actually was. The physical device has 5 buttons, but only 4 of them should work as MultistateInput.

range(1, 6) gives us device triggers for five buttons though. That was also in the original code.
Isn't that one batch of device triggers too many then?
It's also a bit weird, because the original device signature has the MultistateInputCluster on all 5 endpoints (as an output cluster). Could be firmware thing of course, still weird though..

Unfortunately it's not working.

On which HA version did you test the v2 quirk? Did you test it by replacing the whole quirks library or just inserting the one quirks file as a "custom quirk" (using the ZHA config option) (recommended for testing this).
Also, did the quirk apply at least? E.g., in device settings in HA or in the device signature, was there a CustomDeviceV2 quirk used?

mortenmoulder · 2024-10-19T20:20:17Z

@TheJulianJES I am doing the custom quirk with the ZHA config option. It was matched to the signature of one of the devices I tested (the old V1 firmware), but no events were shown when I pressed the buttons.

Also, I appreciate the help. Let me know if there is anything I can do.

OptionHenrik · 2024-10-21T14:25:06Z

@TheJulianJES I am doing the custom quirk with the ZHA config option. It was matched to the signature of one of the devices I tested (the old V1 firmware), but no events were shown when I pressed the buttons.

Also, I appreciate the help. Let me know if there is anything I can do.

@mortenmoulder … was any of your quirk versions working when you added them as custom quirks? I think we are many users waiting for ZHA support to happen and I would like to try deploying a quirk file if your can point to a version you tested to be working.

@TheJulianJES what is missing yet before the PR can be accepted? Hope you will have the time to look at this again. Really appreciate the work you are doing.

mortenmoulder · 2024-10-21T19:32:40Z

@OptionHenrik Yes, the quirk was working perfectly before @TheJulianJES's changes. Feel free to go look at the commit just before his changes, and grab the file from there to give it a go yourself.

We already "skip configuration" during pairing, so we don't need this.

The device only has 5 endpoints. Endpoint 2 to 5 have MultistateInputClusters. This will account for that, as `ENDPOINT_ID: i + 1` with i ranging from 1 to 4.

This isn't really correct, but it's what the original quirk did.

TheJulianJES · 2024-10-21T20:03:10Z

I've made the changes suggested in this comment: #3400 (comment)
So, I've replicated what the original quirk did (removes actual MultistateInput output clusters and inserts our virtual MultistateInput clusters as input clusters. This technically isn't correct, but let's see if that's the issue with the v2 quirk.)

The original PR also created a device trigger for endpoint 6, even though there the device only goes up to endpoint 5.

Please try and see if these changes work now.

TheJulianJES · 2024-10-21T20:05:49Z

zhaquirks/smarthjemmet/quadzigsw.py

+                event_args = {VALUE: value}
+                self.listener_event(ZHA_SEND_EVENT, action, event_args)
+
+            super()._update_attribute(0, action)


If the changes work, please also try and see what these additional changes to this line do:

replacing 0 with MultistateInput.AttributeDefs.present_value.id in this line (sensor entity in HA?)

completely removing this line

I believe this was copied from other quirks where it doesn't actually work, as there is no attribute with id=0 on this cluster.

OptionHenrik · 2024-10-24T20:43:05Z

@OptionHenrik Yes, the quirk was working perfectly before @TheJulianJES's changes. Feel free to go look at the commit just before his changes, and grab the file from there to give it a go yourself.

Works like a charm … great work so far even though it’s only a custom quirk. I hope you guys get it integrated into the standard ZHA soon

TheJulianJES · 2024-10-24T20:45:35Z

Please also test the v2 quirk now. It was updated a few days ago.

radhus · 2024-10-27T20:29:47Z

@mortenmoulder can confirm your quirk works with just a model name change for MULTI-ZIG-SW as well! Great job.

I'll try to find time to test out @TheJulianJES changes later.

mortenmoulder and others added 7 commits October 6, 2024 21:33

Add support for QUAD-ZIG-SW from smarthjemmet.dk

b05cfbe

Apply pre-commit auto fixes

312e1d6

Fix docstrings

9bd1af7

Merge branch 'dev' of https://github.com/mortenmoulder/zha-device-han…

d4eb649

…dlers into dev

Apply pre-commit auto fixes

04baa50

Update file name and add tests

745b05d

Apply pre-commit auto fixes

1170d36

mortenmoulder added 2 commits October 6, 2024 22:22

Fix docstring

df836e2

Merge branch 'dev' of https://github.com/mortenmoulder/zha-device-han…

367ea10

…dlers into dev

TheJulianJES added the new quirk Adds support for a new device label Oct 10, 2024

TheJulianJES added 5 commits October 18, 2024 01:29

Remove empty __init__

65d32f6

Remove unused _current_state

e4fd04f

Use walrus operator for action

c303d4e

Refactor quirk CustomDevice classes

8619f67

Follows the style of other quirks where there's proper quirk defined in a `CustomDevice` class that's inherited from in other quirks that just change the `signature`, but keep the `replacement` and device triggers.

Replace with v2 quirk

d14e974

Remove test

a246d9c

The test here is no longer really needed for the v2 quirk. There's no real point in testing if all the correct clusters are included. Ideally, we would send messages from the device and see if they are converted to "ZHA events" as expected in a test.

TheJulianJES reviewed Oct 18, 2024

View reviewed changes

TheJulianJES added the help wanted Extra attention is needed label Oct 18, 2024

TheJulianJES added 4 commits October 21, 2024 21:51

Remove copied code from LocalDataCluster

1568a8a

We already "skip configuration" during pairing, so we don't need this.

Adjust device trigger range to range(1, 5)

c34f6f2

The device only has 5 endpoints. Endpoint 2 to 5 have MultistateInputClusters. This will account for that, as `ENDPOINT_ID: i + 1` with i ranging from 1 to 4.

Remove output clusters, re-add as input clusters

5143bc1

This isn't really correct, but it's what the original quirk did.

Use AttributeDefs in _update_attribute

544880b

TheJulianJES reviewed Oct 21, 2024

View reviewed changes

TheJulianJES added waiting for reply Waiting for a reply (e.g. to test a custom quirk) and removed help wanted Extra attention is needed labels Oct 26, 2024

TheJulianJES mentioned this pull request Oct 26, 2024

[Device Support Request] smarthjemmet.dk QUAD-ZIG-SW #2846

Open

TheJulianJES linked an issue Oct 26, 2024 that may be closed by this pull request

[Device Support Request] smarthjemmet.dk QUAD-ZIG-SW #2846

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for QUAD-ZIG-SW from smarthjemmet.dk #3400

Add support for QUAD-ZIG-SW from smarthjemmet.dk #3400

mortenmoulder commented Oct 6, 2024 •

edited

Loading

codecov bot commented Oct 6, 2024 •

edited

Loading

mortenmoulder commented Oct 6, 2024

mortenmoulder commented Oct 17, 2024

TheJulianJES commented Oct 17, 2024

TheJulianJES commented Oct 18, 2024 •

edited

Loading

TheJulianJES Oct 18, 2024

mortenmoulder commented Oct 18, 2024

mortenmoulder commented Oct 18, 2024

mortenmoulder commented Oct 18, 2024 •

edited by TheJulianJES

Loading

TheJulianJES commented Oct 18, 2024 •

edited

Loading

mortenmoulder commented Oct 18, 2024

TheJulianJES commented Oct 19, 2024

mortenmoulder commented Oct 19, 2024 •

edited

Loading

OptionHenrik commented Oct 21, 2024

mortenmoulder commented Oct 21, 2024

TheJulianJES commented Oct 21, 2024

TheJulianJES Oct 21, 2024

OptionHenrik commented Oct 24, 2024

TheJulianJES commented Oct 24, 2024 •

edited

Loading

radhus commented Oct 27, 2024

Add support for QUAD-ZIG-SW from smarthjemmet.dk #3400

Are you sure you want to change the base?

Add support for QUAD-ZIG-SW from smarthjemmet.dk #3400

Conversation

mortenmoulder commented Oct 6, 2024 • edited Loading

Proposed change

Additional information

Checklist

codecov bot commented Oct 6, 2024 • edited Loading

Codecov Report

mortenmoulder commented Oct 6, 2024

mortenmoulder commented Oct 17, 2024

TheJulianJES commented Oct 17, 2024

TheJulianJES commented Oct 18, 2024 • edited Loading

TheJulianJES Oct 18, 2024

Choose a reason for hiding this comment

mortenmoulder commented Oct 18, 2024

mortenmoulder commented Oct 18, 2024

mortenmoulder commented Oct 18, 2024 • edited by TheJulianJES Loading

TheJulianJES commented Oct 18, 2024 • edited Loading

mortenmoulder commented Oct 18, 2024

TheJulianJES commented Oct 19, 2024

mortenmoulder commented Oct 19, 2024 • edited Loading

OptionHenrik commented Oct 21, 2024

mortenmoulder commented Oct 21, 2024

TheJulianJES commented Oct 21, 2024

TheJulianJES Oct 21, 2024

Choose a reason for hiding this comment

OptionHenrik commented Oct 24, 2024

TheJulianJES commented Oct 24, 2024 • edited Loading

radhus commented Oct 27, 2024

mortenmoulder commented Oct 6, 2024 •

edited

Loading

codecov bot commented Oct 6, 2024 •

edited

Loading

TheJulianJES commented Oct 18, 2024 •

edited

Loading

mortenmoulder commented Oct 18, 2024 •

edited by TheJulianJES

Loading

TheJulianJES commented Oct 18, 2024 •

edited

Loading

mortenmoulder commented Oct 19, 2024 •

edited

Loading

TheJulianJES commented Oct 24, 2024 •

edited

Loading