Clean up HomeKit accessory information characteristics#14114
Clean up HomeKit accessory information characteristics#14114cdce8p merged 3 commits intohome-assistant:devfrom
Conversation
homeassistant
added
cla-signed
integration: homekit
small-pr
cdce8p
left a comment
cdce8p
left a comment
There was a problem hiding this comment.
Just some small changes.
FYI: The test test_home_accessory fails, since I hard coded the some values.
|
|
||
| def set_accessory_info(acc, name, model, manufacturer=MANUFACTURER, | ||
| serial_number='0000'): | ||
| serial_number=UNKNOWN): |
There was a problem hiding this comment.
What do you think of removing the manufacturer parameter and just assigning the constant below.
The serial_number will always be given, so we don't need UNKNOWN.
There was a problem hiding this comment.
I think we should leave manufacturer for now because I wanted to look into allowing user-specified manufacturer, model, etc. using the HomeKit entity_config.
There was a problem hiding this comment.
We can leave it in there for now, but I don't think this should be done. To be honest I personally what prefer to only those required by HomeKit (SerialNumber) and the the ones necessary to identify the accessory (entity_id either in the Model or like your suggestion in the SerialNumber).
While writing this, is there anything useful you can do with this infos? Why not leave it to just Manufacturer='Home Assistant' and SerialNumber=entity_id?
There was a problem hiding this comment.
According to the Apple HomeKit Developer Guide, the Accessory Information service requires Manufacturer, Model, Name, Serial Number, and Firmware Revision so I think we should keep all of these characteristics for now.
I don't think there's much usefulness from allowing the user to specify the info, more of just a matter of making the user experience more beautiful/correct. My image is that it would be all optional config (i.e. Home Assistant = default manufacturer). I'll experiment with it, if it can be implemented cleanly and without a lot of config, then I'll submit a PR.
There was a problem hiding this comment.
You're right in that they are required chars. However everyone except for Serial Number can be empty strings. In the case of Firmware Revision the char would be hidden if the value is empty.
I will take a look at your PR (regarding the optional config) once it's finished. Have in mind however that every option you add needs to be maintained and therefore is extra work. Sometimes it's best to not do all thats possible.
| """Initialize a Accessory object.""" | ||
| super().__init__(name, aid=aid) | ||
| set_accessory_info(self, name, model=entity_id) | ||
| domain = entity_id.split(".")[0].replace("_", " ").title() |
There was a problem hiding this comment.
Use split_entity_id(entity_id)[0].replace(..... from homeassistant.core instead.
3 participants
Description:
Clean up HomeKit accessory information characteristics. Model is now mapped to
domainand serial number is mapped toentity_id. Also added firmware revision characteristic using current Home Assistant version.Related issue (if applicable): N/A
Pull request in home-assistant.github.io with documentation (if applicable): N/A
Example entry for
configuration.yaml(if applicable):N/AChecklist:
tox. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
REQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices: