Fix use of Matter labels #145652
Fix use of Matter labels #145652
Conversation
|
Hey there @home-assistant/matter, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
@marcelveldt This is a full re-write of my prior attempt to fix the labeling issue. On my last attempt, the code made some guesses about whether to rename using the FixedLabel / UserLabel. You suggested it would be better to be more explicit about when the FixedLabel is used for a entity name. That is done here. This PR builds on your suggest and I do think it is a better approach. Hopefully, you'll agree!. |
| clusters.FixedLabel.Attributes.LabelList, | ||
| clusters.UserLabel.Attributes.LabelList, |
There was a problem hiding this comment.
should we perhaps swap userlabel and fixedlabel so we prefer a userlabel over a fixedlabel ?
There was a problem hiding this comment.
I think FixedLabel shoudl get priority as the primary reason for the label is to give the end-user the manufacturer specified "hint" as to what the endpoint does. The user can then rename it as they please.
The "UserLabel" is secondary. If anything, I was inclined to eliminate the "UserLabel" since it provides no manufacturer "hint" as to purpose of the endpoint, but rather merely contains whatever the user put there (no more useful than renaming by the user), but I kept UserLabel there as it was there in the prior implementation, so I was assuming it was thought to be useful.
For now, its probably irrelevant. I haven't seen any system that allows setting UserLabel yet (I've checked Home Assistant, Apple, Google, SmartThings, Hubitat), so it wouldn't get picked up (and I have not seen the attribute in any device).
| @@ -62,6 +62,8 @@ class MatterEntityDescription(EntityDescription): | |||
| measurement_to_ha: Callable[[Any], Any] | None = None | |||
| ha_to_native_value: Callable[[Any], Any] | None = None | |||
| command_timeout: int | None = None | |||
| # List of Matter FixedLabel or UserLabels used for name | |||
| name_using_matter_labels: list | None = None | |||
There was a problem hiding this comment.
why is this a list and if it needs to be a list, the typing is not complete
There was a problem hiding this comment.
I corrected this to be:
name_using_matter_labels: list[str] | None = None
I think it should be a list. Previously, there was a hard-coded list of two labels ("Label" and "Button"), but there are no standards for how manufacturers should label things. In this iteration, I wanted to have a more adaptable approach, and using a list of the labels allows adapting the list of labels to the device by specifying a list in the discovery schema (which seems less error-prone when it gets changed than changing the main code).
| key="MatterLight", | ||
| name=None, | ||
| name_using_matter_labels=["Label"], |
There was a problem hiding this comment.
is this on purpose not lowercase ?
There was a problem hiding this comment.
Yes, for appearance purposes only, but as explained further in the next comment response, the case doesn't matter as label matching is now case insensitive.
| key="GenericSwitch", | ||
| device_class=EventDeviceClass.BUTTON, | ||
| translation_key="button", | ||
| name_using_matter_labels=["label", "button"], |
There was a problem hiding this comment.
is this on purpose all lowercase?
There was a problem hiding this comment.
I've now changed this to "Label" and "Button" for consistency of how I presented them elsewhere in the code, but as a practical matter, it doesn't matter because ...
The code that does label matching is now case insensitive - both the labels in that name_using_matter_labels list, as well as the FixedLabel / UserLabel values get converted to lower case for the comparison. I did this because the standards don't really give much guidance on how a manufacturer should specify a label - some may be in lower case, upper, mixed, whatever. So I thought it made sense just to be case insensitive.
There was a problem hiding this comment.
FYI - Lines 128-131 of entity.py now implement a case-insensitive match of the labels in "name_using_matter_labels" to the labels in FixedLabel or UserLabel
|
I think the default should be to always prefer the matter labels, because that is exactly where they are intended for. |
I looked at this pretty closely and, it turns out that, more often than not, you don't want to use the label. The reason is that for every cluster, there is generally 1 "controllable" item that you want to label - for example, the main "switch" or "light" control, but then there are several attributes on that same cluster that you don't want to overwrite with the label (e.g., the OnOffTransitionTime, OnTransitionTime, OffTransitionTime, etc.) Since the number of attributes that you don't want to overwrite is larger than the number of control items where you want to use the label, defaulting to not using labels works right the majority of the time. As a side note ... The "original" labeling code was implemented around the Inovelli VTM31 - that device didn't have labels on the RGB endpoint or Dimmer endpoint, and only used labels on the three Generic Switch endpoints. For this device, the fact that the labels were always didn't show up as an issue - the labels were only on the Generic switches and overwriting attribute values didn't happen on those endpoints since the Generic Switch attributes were not used to create Home Assistant entities. In newer products, Inovelli started using labels on the primary control endpoints which made this problem of overwriting attributes for, e.g., level control, fan, and on/Off clusters, more pronounced. Thus, the need to make these changes in labeling.
Here, again, I was finding that due to how different manufacturers use labels, you sometimes don't want to use the label even if the manufacturer gave one, or you only want them on specific entities. As examples, Inovelli uses "Labels" and "Button" and generally those labels have relevant information. Eve uses "orientation" but only on their dual-outlet product. Again, for Eve, the label contains useful information ("top" and "bottom" designation). TP-LINK, on the other hand, used "Position" for their dual outlet plug, but the information there wasn't very useful (just a 1 or a 2), so i that case, I wanted to ignore the label. Gong forward, there is no way to know what will be used by other manufacturers - so as I was doing this rewrite, I wanted to do something flexible and adaptable, and therefore included a means for different discovery schema to differentiate which labels may be used. Thus, the ability to specify a list of label for the entity to be created seemed a way to adapt to particular quirks of the manufacturers implementation. Additionally, in the future, if there was some device that had a really unusual use of labels, you could do a vendor / product specific discovery schema block to handle a particular product with its labeling quirks. |
If you don't agree with my reasons not to always use labels, I think the fix is pretty simple At line 66 of entity.py, change from "None" to have a set of preferred defaults, like: Then in the discovery schema for all of the attributes, mode selects, etc. where you don't want to use labels, include: Or where you want to override the default list, specifying a different list. Changing the default to "always use" with a default list of labels will result in many more changes throughout the discovery schemas as we'll want to indicate that, for all cluster attributes where the attribute name matters (e.g., like for the "onTransition", "offTransition", "onOffTransition" examples) , the code should not use the labels. |
|
There are errors during tests: You should update snapshots files with commands like this and fix tests if needed: New entities names: |
I can try to go back and figure this part out (will be at least a week until I can get the time), but I really don't understand how to setup test, so any help here would be appreciated. |
Check this: |
I tried, but I don't really understand it. |
|
@marcelveldt |
There is a tutorial here: Testing your code. You should update snapshots to fix errors: snapshots update |
|
@jvmahon pytest tests/components/matter/test_button.py --snapshot-update
Test session starts (platform: linux, Python 3.13.3, pytest 8.4.0, pytest-sugar 1.0.0)
rootdir: /home/ludovic/ha_core
configfile: pyproject.toml
plugins: aiohttp-1.1.0, respx-0.22.0, anyio-4.9.0, syrupy-4.9.1, xdist-3.7.0, timeout-2.4.0, requests-mock-1.12.1, unordered-0.7.0, socket-0.7.0, cov-6.1.1, pytest_freezer-0.4.9, github-actions-annotate-failures-0.3.0, picked-0.5.1, sugar-1.0.0, asyncio-1.0.0
asyncio: mode=Mode.AUTO, asyncio_default_fixture_loop_scope=function, asyncio_default_test_loop_scope=function
collected 55 items
tests/components/matter/test_button.py ✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓ 100% ██████████
---------------------------------------------------------------------------------- snapshot report summary ----------------------------------------------------------------------------------
82 snapshots passed. 8 snapshots generated. 8 unused snapshots deleted.
Deleted test_buttons[multi_endpoint_light][button.inovelli_config-entry], test_buttons[multi_endpoint_light][button.inovelli_config-state], test_buttons[multi_endpoint_light][button.inovelli_down-entry], test_buttons[multi_endpoint_light][button.inovelli_down-state], test_buttons[multi_endpoint_light][button.inovelli_up-entry], test_buttons[multi_endpoint_light][button.inovelli_up-state], test_buttons[temperature_sensor][button.mock_temperature_sensor_identify-entry], test_buttons[temperature_sensor][button.mock_temperature_sensor_identify-state] (tests/components/matter/snapshots/test_button.ambr)
Results (19.66s):
55 passed |
Are you saying that, if I go to my branch: https://github.com/jvmahon/HomeAssistantCore/tree/Matter-labeling-fixes and do a "sync fork" that will fix the issue? |
Alternatively, you can create a new branch and copy the changes you've made. This gives you a clean start. |
Update with main branch
|
Breaking change
Proposed change
This pull request fixes several interrelated issues regarding the naming of Matter entities, particularly for multi-endpoint devices.
1. Prevent unintended overwriting of an attribute name using the FixedLabel
2. Improved Association of Attributes for Multi-Endpoint Devices
"Real World" devices that have this problem include Inovelli VTM31 Dimmer, their Fan Switch VTM36 and Inovelli's upcoming Matter Switch (VTM30) which I'm beta testing. Samples of these devices can be sent if needed.
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: