Repository navigation
Conversation
Allows discovery schemas to filter by the Matter ClusterRevision attribute, so entity behavior can differ across cluster spec revisions. Applied to the Thermostat TemperatureOffset number entity, whose LocalTemperatureCalibration constraint changed between Matter 1.3 (±2.5°C) and 1.4 (no explicit spec constraint).
- Drop unused MatterEntityInfo.cluster_revision field and simplify _resolve_cluster_revision() to return a plain bool (Copilot, TheJulianJES) - Fix comment/value inconsistency for the Matter 1.4 TemperatureOffset int8 range (symmetric ±127, not -128..127) (Copilot) - Request matter_node via usefixtures instead of an unused test argument in test_temperature_offset_cluster_revision (Copilot) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ostats The Thermostat TemperatureOffset schema set native_max_value/ native_min_value to raw Matter units (±25) instead of the entity's native unit (°C, after the /10 conversion), letting the UI/service calls send values 10x larger than the underlying int8 attribute storage (LocalTemperatureCalibration) can hold. Use the symmetric int8 storage range instead (±12.7°C, raw ±127), matching what the attribute can actually store. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Keep this PR scoped to the cluster_revision discovery mechanism (const.py/discovery.py/models.py). The number.py TemperatureOffset schema split and its unit fix move to separate PRs (home-assistant#180172 for the unit fix; the revision-based split will follow once that merges). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…y' into matter-cluster-revision-discovery
Covers the cluster_revision_min/cluster_revision_max filtering mechanism directly, since no discovery schema consumes it yet in this PR (the number.py TemperatureOffset split was moved out per review). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace the assert_not_called()/assert_called_once_with() branch with a single call_count assertion driven by parametrize, per review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The unit test for _resolve_cluster_revision() didn't exercise the call site inside async_discover_entities(), so those 3 lines had no coverage from the discovery loop itself (TheJulianJES). Adds a test that patches DISCOVERY_SCHEMAS with a synthetic schema and runs discovery against a real MatterEndpoint built from the mock_thermostat fixture. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Match the wording style of featuremap_contains, per review, so it's clear these check the primary attribute's cluster ClusterRevision. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Rename to _matches_cluster_revision since it only returns a bool, not the revision value (TheJulianJES nit). - ClusterRevision is a non-nullable uint16; the generated cluster dataclass defaults it to 0 when unpopulated, and Matter spec 7.13.1 starts revisions at 1. get_attribute_value() only returns None when the whole cluster is missing, so the None/NullValue check alone never actually caught an unpopulated attribute. Treat 0 the same as unreadable (TheJulianJES). - Extend the parametrized tests to cover the 0 case at both the helper and the async_discover_entities integration level. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Colocate each case's description with its values instead of a separate parallel ids=[] list, per the pytest.param(id=...) guideline. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Also apply the ±12.7°C fix from the generic TemperatureOffset schema to EveTemperatureOffset, since both use the same LocalTemperatureCalibration attribute and int8 storage range. Related: home-assistant#162440 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
With native_step=0.5, the ±12.7°C bounds aren't reachable in even steps from the min value (12.7 / 0.5 = 25.4), which triggers a browser-side step-mismatch validation error. 0.1 divides the range evenly and matches the raw attribute's actual granularity (int8 in 0.1°C units). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Verifies that setting 0.3°C (only reachable with native_step=0.1, not the previous 0.5) correctly writes the scaled raw value to LocalTemperatureCalibration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ure-offset-cluster-revision
…ture-offset-cluster-revision
Older Thermostat cluster revisions (Matter 1.3 and earlier, ClusterRevision <= 6) are spec-limited to ±2.5°C for LocalTemperatureCalibration. Matter 1.4 (ClusterRevision >= 7) extends this to the full int8 storage range of ±12.7°C. Uses the cluster_revision_min/cluster_revision_max discovery filtering from home-assistant#175693 to split the TemperatureOffset and EveTemperatureOffset schemas into a legacy (±2.5°C) and an extended (±12.7°C) variant. When the ClusterRevision can't be read, the legacy schema is matched by default, so the more conservative bound is assumed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
home-assistant
Bot
added
cla-signed
has-tests
integration: matter
new-feature
Top 100
…ffset-cluster-revision # Conflicts: # homeassistant/components/matter/number.py # tests/components/matter/snapshots/test_number.ambr # tests/components/matter/test_number.py
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Both legacy schemas use absolute-temperature conversion for calibration offsets, producing incorrect Fahrenheit values.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Aligns Matter thermostat calibration limits with cluster revisions, building on the discovery-filtering and unit-fix PRs.
Changes:
- Adds cluster-revision discovery filtering.
- Splits generic and Eve offsets into legacy ±2.5°C and extended ±12.7°C ranges.
- Adds range/filter tests and updates snapshots.
| File | Description |
|---|---|
| tests/components/matter/test_number.py | Tests revision-dependent bounds. |
| tests/components/matter/test_discovery.py | Tests revision filtering and unreadable values. |
| tests/components/matter/snapshots/test_number.ambr | Updates legacy range expectations. |
| homeassistant/components/matter/number.py | Splits calibration schemas by revision. |
| homeassistant/components/matter/models.py | Adds revision bounds to discovery schemas. |
| homeassistant/components/matter/discovery.py | Applies revision filtering. |
| homeassistant/components/matter/const.py | Defines the cluster-revision attribute ID. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Use TEMPERATURE_DELTA instead of TEMPERATURE for Matter 1.3 legacy offset schemas. LocalTemperatureCalibration is a calibration offset/delta, not an absolute temperature. Using TEMPERATURE produces incorrect Fahrenheit conversions. This applies to both generic and Eve temperature offset schemas with the ±2.5°C legacy range.
Review feedback addressedFixed both Copilot findings by changing
Commit: 1fed329 |
zigpy-review-bot
left a comment
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Late review, since this was merged before I got to it. The change itself looks right to me and matches what was discussed on #175693: Thermostat ClusterRevision ≤ 6 (Matter 1.3 ships rev 6) gets the legacy ±2.5 °C range, rev 7 and newer get ±12.7 °C. The entity keys are unchanged, so unique IDs stay stable. The Matter number and discovery tests pass locally.
A few small follow-ups:
- The fallback for an unreadable ClusterRevision isn't pinned by any test, and it only works because of schema list order. Details inline.
- #175693 is still open, but its whole diff (
const.py,discovery.py,models.py,test_discovery.py) landed here unchanged, so it can be closed as superseded. - Spec note, no action needed: the Thermostat revision table puts the calibration change at rev 7, but the attribute's own note in the 1.4+ spec says "prior to revision 8". The released data models go from rev 6 (1.3) straight to rev 8 (1.4), so the difference only matters for a device that reports rev 7. The
silabs_water_heaterfixture does, though it has no calibration attribute. Since the revision table names rev 7 as the constraint change, using 7 seems like the right call.
| cluster_revision_min=THERMOSTAT_EXTENDED_CALIBRATION_REVISION, | ||
| ), | ||
| # Matter 1.3 and earlier (ClusterRevision <= 6) limit LocalTemperatureCalibration | ||
| # to ±2.5°C; this schema also matches when ClusterRevision is unreadable. |
There was a problem hiding this comment.
The ±2.5 °C fallback for an unreadable ClusterRevision only works because this schema is listed before the extended one. _matches_cluster_revision() returns True for both schemas when the revision is None/0, and async_discover_entities() keeps whichever comes first for LocalTemperatureCalibration. A missing ClusterRevision attribute reads as the cluster dataclass default 0, so that's the realistic case. The same applies to the two Eve schemas above.
Nothing in the test suite pins this: with both legacy/extended pairs swapped, test_number.py, test_discovery.py and all snapshots still pass, and the unreadable case silently becomes ±12.7 °C. Could a follow-up reword the comment so the ordering requirement is explicit, e.g. "Must stay listed before the extended-range schema: both match when ClusterRevision is unreadable and the first match wins."? It should also add a test case, see the comment on test_number.py.
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("node_fixture", "entity_id", "expected_min", "expected_max"), |
There was a problem hiding this comment.
Suggested follow-up: add the unreadable-revision cases here, so the list-order fallback is pinned. The attributes fixture already feeds matter_node, so it only needs another parametrize column:
@pytest.mark.parametrize(
("node_fixture", "attributes", "entity_id", "expected_min", "expected_max"),
[
# existing two params, each with {} as attributes
pytest.param(
"mock_thermostat",
{"1/513/65533": 0},
"number.mock_thermostat_temperature_offset",
-2.5,
2.5,
id="cluster_revision_unreadable_legacy_range",
),
pytest.param(
"eve_thermo_v5",
{"1/513/65533": 0},
"number.eve_thermo_20ecd1701_temperature_offset",
-2.5,
2.5,
id="eve_cluster_revision_unreadable_legacy_range",
),
],
)I checked this locally: both new cases pass on the merged code and fail when the legacy/extended schema order is swapped.
|
Removed this from the milestone as there's really nothing this fixes from a user point of view. I'll need to take a closer look at this and the bot review comments are all accurate IMO. (EDIT: Looks like this wasn't processed because of the currently ongoing GitHub issues...) |
| def _matches_cluster_revision( | ||
| endpoint: MatterEndpoint, | ||
| primary_attribute: type[ClusterAttributeDescriptor], | ||
| schema: MatterDiscoverySchema, | ||
| ) -> bool: | ||
| """Return whether the endpoint's ClusterRevision matches the schema.""" | ||
| if schema.cluster_revision_min is None and schema.cluster_revision_max is None: | ||
| return True | ||
| raw_cluster_revision = endpoint.get_attribute_value( | ||
| primary_attribute.cluster_id, CLUSTER_REVISION_ATTRIBUTE_ID | ||
| ) | ||
| if raw_cluster_revision in (None, NullValue, 0): | ||
| # ClusterRevision starts at 1 (Matter spec 7.13.1); 0 means the | ||
| # attribute could not be read, so do not filter out the schema | ||
| return True | ||
| cluster_revision_value = int(raw_cluster_revision) | ||
| if ( | ||
| schema.cluster_revision_min is not None | ||
| and cluster_revision_value < schema.cluster_revision_min | ||
| ) or ( | ||
| schema.cluster_revision_max is not None | ||
| and cluster_revision_value > schema.cluster_revision_max | ||
| ): | ||
| return False | ||
| return True |
There was a problem hiding this comment.
Why was this added with this PR? That's #175693 and this PR should not have been marked as ready or reviewed before IMO.
Proposed change
Splits the Matter Thermostat
TemperatureOffset(and vendor-specificEveTemperatureOffset) number entity schemas by the Thermostat cluster'sClusterRevisionattribute, using thecluster_revision_min/cluster_revision_maxdiscovery filtering added in #175693:ClusterRevision <= 6(Matter 1.3 and earlier):LocalTemperatureCalibrationis spec-limited to±2.5°C.ClusterRevision >= 7(Matter 1.4+): the spec limit is lifted, so the entity now uses the fullint8storage range of±12.7°C(the corrected bounds from Fix TemperatureOffset native min/max unit conversion for Matter thermostats #180172).If the
ClusterRevisionattribute can't be read, the legacy (±2.5°C) schema is matched by default, so the more conservative bound is assumed rather than potentially exposing a range the device can't actually honor.As noted in review on #175693:
Dependencies
This PR is stacked on top of two still-open PRs and depends on both:
cluster_revision_min/cluster_revision_maxdiscovery filtering mechanism this PR consumes.TemperatureOffsetnative min/max unit bug and sets the corrected±12.7°Cbound used here for the extended-range schema.Until those merge, this PR's diff against
devalso includes their changes (discovery filtering + unit fix); once they land, the diff will shrink to just the revision-based split shown above.Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: