Replace Inovelli quirks with v2 quirks

[codyhackw]

Proposed change

Updating Inovelli Quirks to the v2 formatting, as well as an update to add the leading/training edge parameter for the VZM36, but removing it from the VZM31SN per Inovelli's request (it's still configurable via button press combo at the switch, which is the recommended method).

Additional information

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

[TheJulianJES]

Oooh, awesome that you're doing this!
There are some very small changes to the quirks v2 API, example here: #3280
It'll land with the next HA beta, so should also be in the final 2024.8.0 release.

Also, there were some repo changes, so you probably need to rebase this (or merge dev into your branch).

[TheJulianJES]

This also seems to supersede this PR then:

• Convert Inovelli blue quirks to v2 quirk [currently a test - DO NOT MERGE] #3020

[codyhackw]

Oooh, awesome that you're doing this! There are some very small changes to the quirks v2 API, example here: #3280 It'll land with the next HA beta, so should also be in the final 2024.8.0 release.

Also, there were some repo changes, so you probably need to rebase this (or merge dev into your branch).

Looks like that was just the change from add_to_registry_v2 to QuirkBuilder being used and add_to_registry() called at the end now? If so, should be fixed now. I believe I chose the right option there on merging dev in, but if not, just let me know and I can get that sorted too.

[TheJulianJES]

Yep, that's fine. Pre-commit just complains about some missing docstrings now.
Tests are a bit different with quirks v2, since we no longer have specific classes for the quirks. I'll have to look at the specific test(s), but you should be able to use device = zigpy_device_from_v2_quirk("manuf", "model") to get a device with the custom clusters in a test.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.44%. Comparing base (79df871) to head (43da1ee).
Report is 6 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #3232      +/-   ##
==========================================
+ Coverage   91.28%   91.44%   +0.16%     
==========================================
  Files         339      340       +1     
  Lines       10967    11177     +210     
==========================================
+ Hits        10011    10221     +210     
  Misses        956      956              

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dmulcahey]

Just comments missing

[TheJulianJES]

And the custom cluster on endpoint 3 isn't replaced yet (when it exists). But we can't add .replaces() for that endpoint yet, since RemovesMetadata assumes the endpoint actually exists (or zigpy crashes otherwise). That seems like an issue.
I'll post more details later and/or fix the zigpy issue if it's one.

[dmulcahey]

And the custom cluster on endpoint 3 isn't replaced yet (when it exists). But we can't add .replaces() for that endpoint yet, since RemovesMetadata assumes the endpoint actually exists (or zigpy crashes otherwise). That seems like an issue. I'll post more details later and/or fix the zigpy issue if it's one.

Ooo… I wonder if we should have another implementation replace_if_present or if we should update the existing one 🤔

[TheJulianJES]

Ooo… I wonder if we should have another implementation replace_if_present or if we should update the existing one 🤔

Up until seconds ago, I thought we can just update the existing one. It adds RemovesMetadata and AddsMetadata.
If we call removes on a non-existent cluster (on a valid endpoint), nothing will happen already. We ignore that case.
But we error out if we try to remove a cluster on a non-existent endpoint.

To ignore this, we can just check if the endpoint exists:
zigpy/zigpy@dev...TheJulianJES:tjj/quirks_v2_removes_non_exist_endpoint (ignore test line change here)

But the above change is not enough. When AddsMetadata is processed, it'll also fail on the non-existent endpoint.
If we also check there that the endpoint exists, a change like this would work: zigpy/zigpy@dev...TheJulianJES:tjj/quirks_v2_removes_non_exist_endpoint_and_add

Currently thinking about if we want to error out when removes, replaces, or adds is called on a non-existent endpoint. Maybe? Like, removes already completely ignores if a cluster that is to be replaced is missing. It just errors out when the endpoint is missing too.
But with adds (also used by `replaces), I'm not sure. We may want to error out here, as the cluster will be missing if the endpoint is missing (obviously). Silently failing here might not be a good option.
But it's also unlikely to ever do this unintentionally? Like, at least when testing, you'll notice that the cluster you're trying to replace/add is missing.

Maybe we want a separate method after all? Not sure yet...

EDIT: We can use the following for this PR:

• Add new quirks v2 metadata type that allows replacing all instances of a cluster on a device zigpy#1441

[TheJulianJES]

I think we should be able to use replace_cluster_occurrences from zigpy/zigpy#1441 for the custom output clusters on all endpoints.

Haven't tested this, but this might work (if zigpy is new enough on the quirks repo, might need to be bumped?):

Suggested change

	.replaces(InovelliVZM31SNCluster, endpoint_id=2, cluster_type=ClusterType.Client)
	.replace_cluster_occurrences(InovelliVZM31SNCluster)

(Replaces both client and server clusters by default. Set replace_server_instances=False to only replace client clusters.)

[github-actions]

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. Thank you for your contributions.

[codyhackw]

just a bump to avoid closing...will get updates together this weekend!

[Copilot]

Pull Request Overview

This PR replaces the legacy Inovelli quirks with the new v2 quirks format using the QuirkBuilder API, while also updating device parameters to meet Inovelli’s latest recommendations. Key changes include:

• Migrating VZM36, VZM35SN, VZM30SN quirks from CustomDevice definitions to chained QuirkBuilder calls.
• Introducing a new v2 quirk for VZM32SN.
• Updating test code to reference the new v2 device factory method for Inovelli quirks.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
zhaquirks/inovelli/VZM36.py	Updates VZM36 quirk using QuirkBuilder with targeted replacements for light and fan clusters.
zhaquirks/inovelli/VZM35SN.py	Replaces legacy quirk with a v2 QuirkBuilder call using cluster occurrence replacement.
zhaquirks/inovelli/VZM32SN.py	Introduces a new v2 quirk definition for the VZM32SN device.
zhaquirks/inovelli/VZM30SN.py	Updates VZM30SN quirk definition with a revised docstring and v2 QuirkBuilder usage.
tests/test_inovelli_blue.py	Adjusts the test to use the new device factory (v2 quirk) and updated cluster mapping.

Comments suppressed due to low confidence (4)

tests/test_inovelli_blue.py:23

• Ensure that the updated test invocation using zigpy_device_from_v2_quirk properly configures the device with the correct cluster mappings for the VZM31-SN quirk.

device = zigpy_device_from_v2_quirk("Inovelli", "VZM31-SN", cluster_ids={2: {0xFC31: ClusterType.Client}})

zhaquirks/inovelli/VZM36.py:15

• Verify that using .replaces() with explicit endpoint_id and cluster_type for the fan cluster is intended and consistent with the desired replacement behavior.

.replaces(InovelliVZM36FanCluster, endpoint_id=2, cluster_type=ClusterType.Client)

zhaquirks/inovelli/VZM35SN.py:9

• Ensure that .replace_cluster_occurrences is the correct method for replacing all instances of InovelliVZM35SNCluster in this device and that its behavior aligns with the new v2 quirk requirements.

.replace_cluster_occurrences(InovelliVZM35SNCluster)

zhaquirks/inovelli/VZM30SN.py:9

• Confirm that replacing the cluster occurrences with InovelliVZM30SNCluster is correctly targeting the intended clusters for the VZM30SN device, given the new v2 quirks approach.

.replace_cluster_occurrences(InovelliVZM30SNCluster)

[TheJulianJES]

More comments on the differences between the v1/v2 quirks.

Do note that some clusters were in INPUT_CLUSTERS (.replaces(cluster) is enough) and others in OUTPUT_CLUSTERS (requires .replaces(cluster, cluster_type=ClusterType.Client) for v2 quirks).

[TheJulianJES]

Identify, OnOff, LevelControl, and InovelliVZM31SNCluster were added for the VZM31-SN (in InovelliVZM31SNv11) on ep 1.
Should we also do that in the v2 quirk? Maybe not..?

[codyhackw]

I think at this point, these can be ignored. That v11 signature would've only been for firmware pre v2.08, and from looking at their site, 2.08 came out way back in November of 2022.

[TheJulianJES]

OnOff, LevelControl, and InovelliVZM31SNCluster were added for the VZM31-SN (in InovelliVZM31SNv10) on ep 1.
Should we also do that in the v2 quirk? Maybe not..?

[codyhackw]

I think at this point, these can be ignored. The v11 signature would've only been for firmware pre v2.08, and from looking at their site, 2.08 came out way back in November of 2022. (this one being v10 would be before that)

[TheJulianJES]

Since the above comment threads are becoming a bit hard to go through, I'll go over the two main points left for this PR:

1. The device type was changed on older firmware versions to correctly create (dimmable) light entities in HA, instead of only switch entity. Relevant comment thread with solution (when zigpy is released + bumped in this repo): Replace Inovelli quirks with v2 quirks #3232 (comment)
   Since newer versions all seem to use the DIMMABLE_LIGHT type, we can just always change that endpoint type to it.

2. Some older quirks adding Identify, OnOff, LevelControl, and InovelliVZM31SNCluster, ... on endpoint 1. These seem crucial to the operation, so I wonder how the device would have worked without presenting them, but since we fixed this with v1 quirks, we should also do this with v2 quirks. And it should be easy:

   We can likely just adds(OnOff.cluster_id) and so on (possibly specifying endpoint_id=x if it's not the default of 1).
   If the clusters are already present, we don't do anything, otherwise, we add them.
   Since newer versions all seem to contain those clusters, there's no reason to not always add them.

   EDIT: Actually, the added clusters were all just output clusters, so we can likely just ignore them..?

[TheJulianJES]

Generated new ZHA diagnostics from the legacy diagnostics provided by @codyhackw (some already with v2 quirks) with the old v1 quirks. Then, re-generated them with the current state of the PR including the .replaces_endpoint(1, device_type=zha.DeviceType.DIMMABLE_LIGHT) patch for VZM31-SN and the newer zigpy version.

The diff between diagnostics from the old v1 quirks and the new v2 quirks is this: TheJulianJES/zha@89b7409 (make sure to load the full diff for all files, though VZM32-SN apparently didn't have a quirk applied before)

That's where I also noticed the different for leading_or_trailing_edge, since it's used on that device.

We should look over the diagnostics to make sure all changes are expected. Some are just caused by reordering of attribute definitions and are fine. Others are caused by v2 quirks no longer removing some Scenes/Group clusters.
There are also some cases where additional attributes have been added.

[codyhackw]

Since the above comment threads are becoming a bit hard to go through, I'll go over the two main points left for this PR:

1. The device type was changed on older firmware versions to correctly create (dimmable) light entities in HA, instead of only switch entity. Relevant comment thread with solution (when zigpy is released + bumped in this repo): [Replace Inovelli quirks with v2 quirks #3232 (comment)](https://github.com/zigpy/zha-device-handlers/pull/3232#discussion_r2106079434)
   Since newer versions all seem to use the `DIMMABLE_LIGHT` type, we can just always change that endpoint type to it.

This has been updated in the most recent merge. Agreed re point 2 with the added clusters being output and think we're fine to leave them as-is.

[TheJulianJES]

This all looks good to me. Thanks again for the effort!

[TheJulianJES]

To also mention this here:

One thing we can consider doing in a future PR is to move the all the device specific clusters (e.g. InovelliVZM31SNCluster, ...) into the device files with the quirk. Since they're all specific to the devices now (and nothing shared between them, other than the base InovelliCluster), it might be a bit cleaner. Especially if v2 quirks define those custom entities in the same file.