ZHA component rewrite part 3 - update helpers #20463
Conversation
ghost
assigned 
| unique_id = None | ||
| if manufacturer and model is not None: | ||
| unique_id = "{}_{}_{}_{}_{}".format( | ||
| slugify(manufacturer), |
There was a problem hiding this comment.
why would we include manufacturer and model in unique ID? Could they otherwise overlap?
There was a problem hiding this comment.
Not really. This keeps them in line with the existing entity Id structure that is already in place. It makes it a lot easier to eyeball debug logs too. I know that isn't the best reason but the entity ids for ZHA are already like this and I figured the consistency and the easier identification in debug logs were worth it.
EDIT The plan is to also make a slight modification to it in the future and use it here So I would like to avoid a breaking change when I do this if it is permissible.
There was a problem hiding this comment.
I say we leave the unique_id based on IEEE + Endpoint (for ZHA profile devices) or IEEE + Endpoint + Cluster_ID (for zha single cluster devices). My opinion: it is fine for entity_id to include manufacturer/model etc, but unique ID should be left as it was prior.
And it also going to be a big breaking change. I'd rather rename this into construct_entity_id()
There was a problem hiding this comment.
I can do ieee + endpoint + cluster
To be clear this is currently only used in listeners (later PR). I did it this way to align them with entity ids so the logic could be shared in the future but I can def change to what is suggested above. This will have nothing to do with entity ids at that point so the name won’t change
There was a problem hiding this comment.
I just didn't want this to become a HA entity unique_id. I guess no restrictions for use inside the listeners, but as you mentioned it does rise a question of easily identifiable names in listeners debug output.
If we go by the premise that listeners are more closely related to Zigpy, have you dismissed idea of using the [0xNWK:EP_ID:Cluster_ID] format for logging, same as it is in Zigpy?
There was a problem hiding this comment.
Nope. Was just making sure the intent was clear. I’ll align on that it will make our lives a lot easier!
There was a problem hiding this comment.
We should never change unique IDs. It gives a really bad user experience, as all entities will get new entity IDs, as their old ones are reserved for their previous unique IDs.
There was a problem hiding this comment.
So if this is new code, leave the unique ids.
There was a problem hiding this comment.
All new and not anything that will affect the users. This is to identify different listener implementations (coming in future PR) and they have no tie to entities. I was going to unify them in a non breaking fashion later but we decided against it. I can rename this to listener_id to avoid confusion in the future if that helps.
|
@balloob are these updates good? |
This PR updates the helpers module. This will minimize noise in the dif for the important PR's that do the real work. See #20434 for details and reasoning.