Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UniqueID should probably not be a mandatory attribute #173

Closed
kjagiello opened this issue Jan 16, 2023 · 6 comments
Closed

UniqueID should probably not be a mandatory attribute #173

kjagiello opened this issue Jan 16, 2023 · 6 comments

Comments

@kjagiello
Copy link

kjagiello commented Jan 16, 2023

Want to open with saying that I'm completely new to matter, so I might be misunderstanding the spec 😅 But hopefully I'm on the right track here!

I have encountered an issue when commissioning my ESP32 C3 running the light example from esp-matter (on recent commit, CHIP v1.0.0.2). Commissioning using the latest HASS results in the following stacktrace:

 (...)
  File "/usr/src/homeassistant/homeassistant/components/matter/entity.py", line 67, in __init__
    f"{node.unique_id}-"
  File "/usr/local/lib/python3.10/site-packages/matter_server/common/models/node.py", line 215, in unique_id
    self.get_attribute(
  File "/usr/local/lib/python3.10/site-packages/matter_server/common/models/node.py", line 152, in get_attribute
    return next(
StopIteration

It seems that others are affected by it as well:

While reading the spec (22-22349-001_Matter-1.0-Core-Specification.pdf, section 11.1.6.3), I have noticed that the Unique ID attribute within Basic Information is not mandatory, but optional, which does not seem to align with the getter here:

@property
def unique_id(self) -> str:
"""Return uniqueID for this node."""
return cast(
str,
self.get_attribute(
self.root_device_type_instance.endpoint, Clusters.Basic, "UniqueID"
).value,
)

Could it be that a misassumption has been made here about this attribute?

@kjagiello kjagiello changed the title UniqueID is not a mandatory attribute UniqueID should probably not be a mandatory attribute Jan 16, 2023
@agners
Copy link
Collaborator

agners commented Jan 16, 2023

Agreed, we cannot rely on this attribute to be present.

Also, even if its mandatory, it does not guarantee to remain static across factory reset, from the standard:

It MAY be constructed using a permanent device identifier (such as device MAC address) as basis.
In order to prevent tracking,
• it SHOULD NOT be identical to (or easily derived from) such permanent device identifier
• it SHOULD be updated when the device is factory reset
...

Maybe using Node ID or something which derives from it (e.g. Operational Instance Name) as unique ID? The Operational Instance Name would be nice since this is used as DNS‐SD instance name.

@kjagiello
Copy link
Author

kjagiello commented Jan 16, 2023

Yeah, based on what I'm reading in the core spec, it looks like Node ID fits the bill. I guess Operational Instance Name would fit the bill too, since it consists of both Fabric ID and Node ID.

Update: I have implemented a dirty fix (Node ID returned from .unique_id) locally and it seems to have solved my issue. The device is now successfully recognized by HASS and correctly re-associated even after rebooting.

@spartandrew18
Copy link

Is there a way to dirty fix this for the eve devices in the matter addon/integration?

@marcelveldt
Copy link
Contributor

Unique ID was mandatory in a pre-v1 stage but is now optional and it looks like most implementations abandoned it.
We are going to provide a small quick fix to switch from unique id to the node id. Unfortunately there is no 100% solid way (yet) to detect a device inter-fabric or between (re)commissions. I guess that is a problem for later :-)

@kjagiello
Copy link
Author

Oh, that explains it! Thanks!

@marcelveldt
Copy link
Contributor

marcelveldt commented Jan 16, 2023

Fix is PR-ed for HA core. We'll remove the property within the lib later as we have a small refactor of the node class scheduled.

home-assistant/core#86046

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants