Drop Inovelli attribute init entries the quirk already covers - #863
Drop Inovelli attribute init entries the quirk already covers#863TheJulianJES wants to merge 1 commit into
Conversation
zha-quirks #5122 ported the Inovelli entities to quirks v2 and #802 removed their ZHA-native counterparts, but the `Inovelli*Init` virtual entities kept the full attribute lists they were written against. Each quirk entity now declares its own startup read, so most of those entries are dead weight: 90 of 119 across the three models. Remove exactly the entries whose quirk entity produces an identical `AttrConfig`. Two groups have to stay, because nothing on the quirks side reproduces them: - attributes with no quirk entity (e.g. `power_type`, the power/energy report thresholds, the VZM35 non-neutral aux learn values), and - attributes ZHA reads fresh from the device where the quirk entity initializes from the cache — `QuirkBuilder` defaults `attribute_initialized_from_cache=True` and the Inovelli builder never overrides it, so `button_delay`, `local_protection`, `output_mode`, `switch_type`, `smart_bulb_mode`, `smart_fan_mode` and the VZM35 `quick_start_time` would silently become cache-only reads. `read_on_startup` merges with `or` across entities, so dropping an entry whose quirk config matches is a no-op: the merged per-attribute config is unchanged for all three models (43, 46 and 41 attributes respectively). `InovelliBind`/`InovelliClientBind` also stay — no Inovelli quirk uses `reporting_config`, so nothing binds 0xFC31 on the quirks side.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #863 +/- ##
=======================================
Coverage 97.15% 97.15%
=======================================
Files 55 55
Lines 10481 10481
=======================================
Hits 10183 10183
Misses 298 298 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zigpy-review-bot
left a comment
zigpy-review-bot
left a comment
There was a problem hiding this comment.
No blockers. I reproduced the behaviour-no-op claim mechanically rather than by re-reading the tables, and it holds exactly: the merged per-attribute config for cluster 0xFC31 is byte-identical before and after on all four Inovelli snapshots in tests/data/devices/. One suggestion (a regression test for the cross-repo invariant) and one docstring nit.
How I checked. I dumped aggregate_cluster_configs(zha_device._discovered_entities) for inovelli-vzm30-sn, inovelli-vzm30-sn-0x01100100, inovelli-vzm31-sn-0x01020212 and inovelli-vzm35-sn-0x02020107 at origin/dev and at d07fffc, and diffed the resulting {attr_name: read_on_startup} maps. Identical on all four, including bind and the contributing-entity sets. The totals match your table (43 / 46 / 41 attributes read on startup, of which 6 / 6 / 7 fresh), and 36→9, 46→11, 37→9 entries = the 90-of-119 you quote.
I also verified the two retained groups per-model against zha-quirks 2.2.0 rather than taking the comments at face value, and every retained entry is load-bearing:
- "no quirk entity" — correct for all 14. Worth noting the asymmetries are real, not oversights: the VZM30-SN quirk declares neither
switch_typenorquick_start_time, the VZM31-SN quirk declares.inovelli_switch_type()but noquick_start_time, and the VZM35-SN quirk declares both (.inovelli_fan_switch_type()/.inovelli_quick_start_time()) — which is exactly whyswitch_typesits in group 1 for the VZM30-SN and group 2 for the other two, andquick_start_timein group 1 for the VZM31-SN and group 2 for the VZM35-SN. None of these are firmware-conditional, so the grouping isn't snapshot-specific. - "quirk entity initializes from cache" — correct for all 15: each maps to a quirk entity contributing
read_on_startup=False, andzhaquirks/inovelli/builder.pynever passesattribute_initialized_from_cache=False. SinceAggregatedAttrConfig.mergeors, these entries are also safe in both directions — if the quirk later flips that default they become redundant but harmless, and if the quirk ever drops one of those entities the fresh read survives here.
Suggestion — nothing in CI holds the invariant this rests on. The whole correctness argument is "the quirk reproduces these entries", and that argument currently lives only in the PR body and two source comments. There is no test anywhere in tests/ that touches aggregate_cluster_configs or read_on_startup, and the device snapshots record zha_lib_entities (entity shape) only — no cluster config — so test_devices_from_files won't notice either. If a future zha-quirks release renames one of these attributes, drops one of the ~90 entities, or reorders which entity carries the startup read, ZHA silently stops reading those attributes at startup and every ZHA test stays green.
A small parametrized test over the three snapshots asserting the merged {attr_name: read_on_startup} map for 0xFC31 would turn the table you computed by hand into a CI invariant — essentially the probe I ran above, as a fixture. It'd pay for itself twice over given #5128 / #5171 will each re-run the same by-hand analysis on the remaining 29 entries, and it also protects the group-2 entries, which are the most fragile category here (they exist purely to override a quirks-side default and read like dead weight to anyone doing a later cleanup pass).
Not asking you to retrofit it for the other vendors' *Init classes — the Inovelli three are the ones actively being whittled down.
One observation, no action needed here. zha-quirks #5122 first shipped in 2.2.0, but pyproject.toml still floors at zha-quirks>=2.0.0 (unchanged since #762). On 2.0.0/2.1.x these attributes would now have no reader at all. #802 already created that exposure so it isn't introduced here, but the failure mode gets quieter with this PR — #802 loses visible entities, this one silently leaves config values stale. Might be worth raising the floor separately.
Verified (7 checks)
- Aggregated
0xFC31config diffedorigin/devvsd07fffcon all four Inovelli snapshots — identical (attributes,read_on_startup,bind, contributing entities). - Per-entity contribution breakdown confirming each retained entry's group, against installed zha-quirks 2.2.0.
AggregatedAttrConfig.merge(zha/zigbee/cluster_config.py:39) orsread_on_startup;initialize_cluster_configssplits intocached_attrs/fresh_attrs, so an attribute lands in exactly one bucket and fresh wins — theorargument is sound.- Static cross-check of
zhaquirks/inovelli/{builder,VZM30SN,VZM31SN,VZM35SN}.pyforswitch_type/quick_start_time/attribute_initialized_from_cache; no firmware filters on any of them. grepovertests/foraggregate_cluster_configs/read_on_startup: zero hits.tests/test_discover.py(892 tests) passes locally on the PR head;ruff checkandruff format --checkclean.- Copilot (GPT-5.5) second opinion, scoped to the three classes and the cross-repo claims: no findings.
| """Inovelli VZM30-SN switch attribute init.""" | ||
| """Inovelli VZM30-SN switch attribute init. | ||
|
|
||
| Only attributes the quirk does not already read on startup. See |
There was a problem hiding this comment.
Nit: "does not already read on startup" isn't quite right for the second group — the quirk does read button_delay / smart_bulb_mode / local_protection / output_mode on startup, just from the cache. What this class still contributes for those is the fresh read, not the read itself.
Suggest "…the quirk does not already read fresh from the device on startup", or just dropping the first sentence and leaving the pointer to InovelliVzm31Init, whose docstring states both groups precisely. Same wording on InovelliVzm35Init (line 1149).
| "internal_temp_monitor": False, | ||
| "overheated": False, | ||
| "power_type": True, | ||
| # quirk entity initializes from cache, we want a fresh read |
There was a problem hiding this comment.
These four are the entries most likely to be wrongly deleted by a later cleanup pass: unlike the group above, the attribute is covered by a quirk entity, so the only thing distinguishing them is a default (attribute_initialized_from_cache=True) that lives in zhaquirks/inovelli/builder.py and isn't visible from here.
That's the strongest argument for the regression test in the summary — an assertion on the merged {attr_name: read_on_startup} map for 0xFC31 is the only thing that would catch either a quirks-side change or an over-eager future deletion.
2 participants
This removes some unnecessary attribute initialization for Inovelli virtual entities. With either of the following future PRs, we should be able to drop most of the rest as well:
AI summary
zigpy/zha-device-handlers#5122 ported the Inovelli entities to quirks v2 and #802 removed their ZHA-native counterparts, but the
Inovelli*Initvirtual entities kept the full attribute lists they were written against. Each quirk entity now declares its own startup read, so most of those entries are dead weight: 90 of 119 across the three models.This drops exactly the entries whose quirk entity already produces an identical
AttrConfig, which makes it a behaviour no-op —AggregatedAttrConfig.mergecombinesread_on_startupwithor, so removing an entry the quirk reproduces cannot change the merged result. Checked per model by computing the merged per-attribute config from the quirk metadata plus the ZHA list, before and after:The 29 entries that stay, and what it would take to drop them (CLICK TO EXPAND)
Group 1 — no quirk entity at all (14 entries)
Nothing on the quirks side reads these, so these entries are the only reason the values are ever fetched.
active_power_reports,periodic_power_and_energy_reports,active_energy_reports,power_type,switch_typeactive_power_reports,periodic_power_and_energy_reports,active_energy_reports,quick_start_time,quick_start_level,power_typenon_neutral_aux_med_gear_learn_value,non_neutral_aux_low_gear_learn_value,power_typeDropping these needs a quirks v2 way to read an attribute once on startup without an entity and without reporting. As currently drafted, both #5128 and #5171 take
reporting_config: ReportingConfigas a required argument toconfigures_reporting, soread_on_startup=Trueis only reachable for an attribute that is also set up for reporting. A plain entity-less read looks like the one piece neither covers yet.Group 2 — quirk entity exists, but initializes from cache (15 entries)
QuirkBuilderdefaultsattribute_initialized_from_cache=Trueandzhaquirks/inovelli/builder.pynever overrides it, so #5122 did not carry ZHA's fresh-read intent across. Drop these and the attributes silently become cache-only reads.button_delay,smart_bulb_mode,local_protection,output_modeswitch_type,button_delay,smart_bulb_mode,local_protection,output_modeswitch_type,quick_start_time,button_delay,smart_fan_mode,local_protection,output_modeThis group needs no new API: passing
attribute_initialized_from_cache=Falsefor those entities inzhaquirks/inovelli/builder.pyis enough, and can land independently of both PRs above.(
switch_typeis in group 1 for the VZM30-SN, which has no quirk entity for it, and in group 2 for the other two models.)InovelliBind/InovelliClientBindThese stay too. No Inovelli quirk uses
reporting_config, so nothing binds0xFC31on the quirks side today — these two are what make the device send its unsolicited button/LED reports. They are covered by.binds()(#5128) /.binds_cluster()(#5171).