Import device diagnostics with a firmware version filename suffix#799
Conversation
Make importer more robust
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #799 +/- ##
=======================================
Coverage 97.29% 97.29%
=======================================
Files 55 55
Lines 10934 10934
=======================================
Hits 10638 10638
Misses 296 296 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
TheJulianJES
left a comment
TheJulianJES
left a comment
There was a problem hiding this comment.
This is a nice improvement!
| if attr_id_int in real_cluster.attributes: | ||
| real_cluster._attr_cache[attr_id_int] = parsed | ||
| else: | ||
| real_cluster._attr_cache.set_legacy_value(attr_id_int, parsed) | ||
| real_cluster.PLUGGED_ATTR_READS[attr_id_int] = parsed |
There was a problem hiding this comment.
Should we log when we set legacy values? I think they should almost never be used these days (except for maybe the Aqara P1 quirk).
(In some future PR, maybe we can even have regeneration + diagnostics import print file name + what warnings occurred as a summary, but idk.)
(Also related issue of attr ID collisions: #798. I fear that we may deviate from real devices too much at some point)
There was a problem hiding this comment.
There are a few hundred 😓 66620ab (this PR)
There was a problem hiding this comment.
Oh... I guess we should look into what those unmapped attributes are 😅 (zigpy/zha-device-handlers#5120)
| if ( | ||
| ha_data.get("last_seen") == "2026-03-04T18:21:39.038488+00:00" | ||
| ) and (ha_data.get("model") == "J1 (5502)"): | ||
| _LOGGER.debug("Skipping known-bad DIY device") | ||
| continue |
There was a problem hiding this comment.
Should we have some constant(s) for bad imports? But I guess we hopefully don't need to expand this list ever 😅
zigpy-review-bot
left a comment
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Approve — test-infrastructure + test-data only.
No runtime zha/ code is touched (changes are confined to tools/ and tests/), so there's no entity-discovery or unique_id risk for existing users. I verified the snapshot diff matches the methodology in the description: 454 byte-identical renames (just the -0x… firmware-version suffix), 43 new fixtures, 0 deletions, and 12 content-modified files. The 12 modified ones are the devices that report no firmware version (so they keep their old filename); their unique_id churn is the deterministic faked IEEE (ieee_from_manufacturer_model) changing on re-import, not a discovery regression. CI is green across 3.12/3.13/3.14 and the affected test_discover/test_alarm_control_panel/test_select/test_switch suites pass locally (890 tests).
Two optional notes on the import tool inline — neither blocks.
Non-blocking, and out of scope here: tests/test_inovelli.py.bak is tracked and carries stale references, but it's pre-existing (from #762) and .bak isn't collected by pytest, so it's inert. Might be worth a git rm in a separate cleanup.
| app=zha_gateway.application_controller, | ||
| device_data=data, | ||
| ) | ||
| except Exception: |
There was a problem hiding this comment.
Optional: this except Exception: makes the loader-dispatch swallow real failures. Since native device fixtures have no home_assistant key, a native-format input that fails to parse for a genuine reason hits this branch, matches "home_assistant" not in data, and is skipped with the misleading "Skipping, missing 'home_assistant' key" debug line — the original error is lost.
Cleaner would be to dispatch on the input shape up front (home_assistant key present → HA-diagnostics loader; absent → native loader) so there's no broad except at all. At minimum, log the caught exception (_LOGGER.debug("modern parse failed for %s", path, exc_info=True)) before falling back.
| continue | ||
| ha_data = data["home_assistant"].get("data", {}) | ||
| if ( | ||
| ha_data.get("last_seen") == "2026-03-04T18:21:39.038488+00:00" |
There was a problem hiding this comment.
Optional: skipping the known-bad device by a hardcoded last_seen timestamp + "J1 (5502)" model is brittle. A one-line comment on why this device fails to import would help whoever trips over it next.
puddly
force-pushed
the
puddly/diagnostics-fw-version
branch
from
66620ab to
8bda80b
Compare
This PR appends the firmware version string to the diagnostics filename, allowing us to test version-gated quirks.
I built this PR in a specific way to ensure that the recent ZHA rewrite doesn't have any entity-level regressions:
devwas mergedThe last step succeeded with just
quirk_classrenames.