-
Notifications
You must be signed in to change notification settings - Fork 43
Additional support for components #77
base: dev
Are you sure you want to change the base?
Conversation
5267cb5
to
250c65e
Compare
@andrewsayre I believe this is ready for review, thanks! |
@@ -228,6 +227,13 @@ def values(self) -> Dict[str, Any]: | |||
lambda: None, {k: v.value for k, v in self._attributes.items()} | |||
) | |||
|
|||
@property | |||
def disabled_components(self) -> []: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is from the custom
namespace, which means this capability has no formal definition and can be changed by any device implementing it. Meaning, it's not guaranteed to be a dictionary of strings (like you're expecting) and will result in runtime errors if assumed to be so. Given the nature of custom capabilities, they don't below as first class members in this library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed your comment on the pr for home assistant, if I understood well the idea is that such functionality is implemented on the ha side and not the library correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's my question too; would you accept this moved to ha/core, assuming we check the data type first? Or alternatively check the data type here which would be simpler, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All methods updating status (like switch_off) needs to take to account component_id in the parameter, because it updates always state of the "main" component, not a correct one. For example if you swith off some component (not the main), it switchs off also main component in device state (it is not really switched off in the smart things device).
I was wondering if there's any update on this. I was hoping to control my heat pump via home assistant and this seems to be required for it to work. Thanks a bunch! |
andrew has not been active to give insights on the open question. Same goes for the open smartthings pull requests in the HA repository. Seems smartthings is kind of in need of a new maintainer. |
Just in case, here is a fixed (at least for me) SmartThings you can install via HACS: https://github.com/contemplator1998/smartthings (Pulled home-assistant/core#99924 and wrapped into a HACS repo) It makes everything accessible, even sensors/controls that are always unavailable (just disable them in the HA). |
with that approach it might introduce unwanted capabilities, to support all components with all commands the code should be bit more flexible, i have no issue to implement it but the question is whether @andrewsayre will approve it. just to explain what I thought: the pros/cons of the generic approach is that in HA all components will be supported also for newly added devices and capabilities, cons of it - bit less readable in terms of code (generic terms), samailir to any hub (zigbee2mqtt, xiaomi, etc...) @andrewsayre , please let me know if it worth the investment |
@elad-bar can you elaborate further? How would mapping to more complex platforms (e.g. climate) work? For example, there is conditional logic to map groups of capabilities to platform features and maps of attribute values to states. |
I'm working on a seperate python project (will share when it is ready) based on the api, |
You can have a look into another integrations i contributed similar functionality: Of course for that integration it would be "bit" more challenging as it has more complex entities, key is to extend the EntityDesceiption of HA to be able to deal with it |
I found out the presentation api that supposed to make it easier, for each device it allows to get the relevant capabilities to construct complex ui components including actions and automation |
investigated a bit the api, below is the json of the response: {
"dashboard": {
"states": [
{
"label": "{{heatingSetpoint.value}} {{heatingSetpoint.unit}}",
"alternatives": [
{
"key": "C",
"value": "°C",
"type": "active"
},
{
"key": "K",
"value": "°K",
"type": "active"
},
{
"key": "F",
"value": "°F",
"type": "active"
}
]
}
],
"actions": []
},
"detailView": [
{
"label": "{{i18n.label}}",
"displayType": "slider",
"slider": {
"range": [
0.0,
40.0
],
"unit": "heatingSetpoint.unit",
"supportedValues": "heatingSetpointRange.value",
"command": "setHeatingSetpoint",
"argumentType": "number",
"value": "heatingSetpoint.value",
"valueType": "number"
}
}
],
"automation": {
"conditions": [
{
"label": "{{i18n.label}}",
"displayType": "numberField",
"numberField": {
"value": "heatingSetpoint.value",
"valueType": "number",
"unit": "heatingSetpoint.unit",
"range": [
0.0,
40.0
],
"supportedValues": "heatingSetpointRange.value"
}
}
],
"actions": [
{
"label": "{{i18n.commands.setHeatingSetpoint.label}}",
"displayType": "numberField",
"numberField": {
"command": "setHeatingSetpoint",
"argumentType": "number",
"unit": "heatingSetpoint.unit",
"range": [
0.0,
40.0
],
"supportedValues": "heatingSetpointRange.value"
}
}
]
},
"id": "thermostatHeatingSetpoint",
"version": 1
} it is possible also to define new presentation per app, for instance, for HA smart thing app (has its own app id), then, it will be easier to translate the displayType to specific HA component. |
@andrewsayre pls review the repo i released my poc, didn't want to push it as PR here.
|
@andrewsayre did you manage to review my repo with poc? thanks |
Is there a working plugin for SmartThings yet? I know the official one has been broken forever, there is a HACS version that finally shows the temperatures of Samsung Fridges, but loses the ability to show power consumption (the only thing that works with the official version). If anyone has a working plugin (HACS or not) please ping me. Thank you! |
Description:
Small changes to assist in supporting components in the home assisstant integration.
It's nicer to be able to loop through all the components instead of having to treat main as different. I don't think this will break anyone else's existing use since it's just adding something.
Also added a convenience method to get the list of disabled components, if available.
This PR is required for home-assistant/core#99924
Related issues:
#44
home-assistant/core#91892
Checklist:
.coveragerc
allowedREADME.MD
updated (if necessary)