Add ST218 (Zigbee Thermostat) support#10495
Conversation
There was a problem hiding this comment.
line too long (87 > 79 characters)
There was a problem hiding this comment.
line too long (81 > 79 characters)
There was a problem hiding this comment.
line too long (87 > 79 characters)
There was a problem hiding this comment.
line too long (83 > 79 characters)
There was a problem hiding this comment.
closing bracket does not match visual indentation
There was a problem hiding this comment.
line too long (83 > 79 characters)
There was a problem hiding this comment.
line too long (87 > 79 characters)
There was a problem hiding this comment.
line too long (87 > 79 characters)
There was a problem hiding this comment.
You probably meant "climate device" here. ;)
There was a problem hiding this comment.
These should be constants, preferrably taken from the STATE_* constants in climate/init.py
There was a problem hiding this comment.
Why do you set this only to translate it later into TEMP_CELSIUS? Can't you directly set TEMP_CELSIUS here?
There was a problem hiding this comment.
As far as I can see, self._unit is set exactly once, on initialization, and always to "C". So this should always return TEMP_CELSIUS? Is self._unit still necessary then?
There was a problem hiding this comment.
According to https://home-assistant.io/components/climate/ , this call can also be used to set operation mode and temperature in one go. Your method seems to ignore the operation mode.
There was a problem hiding this comment.
continuation line under-indented for visual indent
There was a problem hiding this comment.
expected 1 blank line before a nested definition, found 0
at least two spaces before inline comment
There was a problem hiding this comment.
'homeassistant.const.TEMP_FAHRENHEIT' imported but unused
There was a problem hiding this comment.
'homeassistant.components.climate.STATE_HEAT' imported but unused
There was a problem hiding this comment.
at least two spaces before inline comment
MartinHjelmare
left a comment
MartinHjelmare
left a comment
There was a problem hiding this comment.
We shouldn't merge this until the required PR in bellows is merged.
There was a problem hiding this comment.
This dict could be moved to the module level as a constant.
There was a problem hiding this comment.
Same as above for this list.
There was a problem hiding this comment.
If you know that the key is in the dict, don't use dict.get(key), just access the key directly, dict[key].
There was a problem hiding this comment.
Cache the operation mode to avoid two lookups.
operation_mode = kwargs.get(ATTR_OPERATION_MODE)
if operation_mode is not None:
yield from...There was a problem hiding this comment.
Same as above. Also invert the check.
if temperature is None:
return
yield from...There was a problem hiding this comment.
I'd extract these magic bytes into constants.
There was a problem hiding this comment.
You still set the operation, even if it is not a possible operation - is that intended? Have you tested what that does to the front end? I guess no operation mode will be shown in the front end in this case.
There was a problem hiding this comment.
Better use dict.get here like it was before. Then it will return None if raw_mode is missing in OPERATIONS.
There was a problem hiding this comment.
This probably fails if there are multiple requests "in the queue": Say I call a service that requires communication three times simultaneously. The first one goes through and sets last_request to the current time. The next two requests both sleep for one second and then perform their request within the same second (if the machine is fast enough). I think you need to loop here, calling asyncio.sleep until time.time() - self.last_request >= CLEAN_TIME_ZONE becomes true.
There was a problem hiding this comment.
Maybe you could add a lock that is acquired before entering the sleep and released after the sleep?
There was a problem hiding this comment.
I think this should at least print an error message if a read fails.
There was a problem hiding this comment.
Couldn't we also pinpoint the exception to eg OSError?
There was a problem hiding this comment.
This throws if raw_mode not in OPERATIONS. Is that intended? I don't think so, as that also causes the temperatures not to be updated.
There was a problem hiding this comment.
Maybe just missing else statement
There was a problem hiding this comment.
How can self._target_temperature < 0 happen? Is that used as error indicator somewhere? I can't find it.
tinloaf
left a comment
tinloaf
left a comment
There was a problem hiding this comment.
See inline code comments
|
|
||
| SUPPORT_FLAGS = SUPPORT_TARGET_TEMPERATURE | SUPPORT_OPERATION_MODE | ||
|
|
||
| @asyncio.coroutine |
| """ | ||
| Climate on Zigbee Home Automation networks. | ||
|
|
||
| For more details on this platform, please refer to the documentation |
There was a problem hiding this comment.
Please add in this description the model and manufacturer supported.
| # Clean data | ||
| for data in ('manufacturer', 'model'): | ||
| if isinstance(discovery_info.get(data), bytes): | ||
| discovery_info[data] = discovery_info[data]\ |
There was a problem hiding this comment.
Please add a debug log with raw data. Useful if the manufacturer changes message format and you don't have a device at hand to debug.
| discovery_info[data] = discovery_info[data]\ | ||
| .split(b'\x00')[0].decode('utf-8') | ||
| elif isinstance(discovery_info.get(data), str): | ||
| discovery_info[data] = discovery_info[data].split('\x00')[0] |
There was a problem hiding this comment.
Please add a debug log with raw data. Useful if the manufacturer changes message format and you don't have a device at hand to debug.
| class ClimateST218(zha.Entity, ClimateDevice): | ||
| """Climage Stelpro ST218.""" | ||
|
|
||
| _domain = DOMAIN |
| for result in res: | ||
| for subresult in result: | ||
| if subresult.status != NO_ERROR_ANSWER_CODE: | ||
| _LOGGER.error("Error setting operation mode: %s", |
There was a problem hiding this comment.
Is this message correct? Seems you're showing operation mode message for result of temperature set.
| OPERATIONS = {0: STATE_OFF, | ||
| 4: STATE_COMFORT, | ||
| 5: STATE_ECO} | ||
| REVERSE_OPERATIONS = dict((v, k) for k, v in OPERATIONS.items()) |
There was a problem hiding this comment.
Is REVERSE_OPERATIONS used?
There was a problem hiding this comment.
Maybe just missing else statement
| if operation_mode is not None: | ||
| yield from self.async_set_operation_mode(operation_mode) | ||
|
|
||
| temperature = kwargs.get(ATTR_TEMPERATURE) |
There was a problem hiding this comment.
Maybe name this raw_temp to be consistent before applying transformation
|
Bellows PR has been merged. What is the status of this PR? |
|
@balloob The PR was closed, the bellows devs are currently refactoring it. BTW, Thanks for your work ! |
ghost
added
integration: const
integration: zha
and removed
platform: climate.zha
labels
8 participants
Description:
Add ST218 (Zigbee Thermostat) support
Could be cleaned when zigpy/bellows#71 is merged
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#3971
Checklist:
If user exposed functionality or configuration variables are added/changed:
toxrun successfully. Your PR cannot be merged unless tests passREQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.