Homekit: Exception on set cover position

[nickw444]

Home Assistant release with the issue:

0.66.1

Last working Home Assistant release (if known): N/A

Operating environment (Hass.io/Docker/Windows/etc.):

Homekit on iOS 11.3

Component/platform:

Homekit

Description of problem: When attempting to open/close a cover via Homekit on iPhone, a traceback occurs and the homekit server stops responding.

Problem-relevant configuration.yaml entries and (fill out even if it seems unimportant):

homekit:

Traceback (if applicable):

Exception happened during processing of request from ('192.168.8.179', 49201)
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/socketserver.py", line 639, in process_request_thread
    self.finish_request(request, client_address)
  File "/usr/local/lib/python3.6/site-packages/pyhap/hap_server.py", line 805, in finish_request
    self.RequestHandlerClass(sock, client_addr, self, self.accessory_handler)
  File "/usr/local/lib/python3.6/site-packages/pyhap/hap_server.py", line 141, in __init__
    super(HAPServerHandler, self).__init__(sock, client_addr, server)
  File "/usr/local/lib/python3.6/socketserver.py", line 696, in __init__
    self.handle()
  File "/usr/local/lib/python3.6/http/server.py", line 420, in handle
    self.handle_one_request()
  File "/usr/local/lib/python3.6/http/server.py", line 406, in handle_one_request
    method()
  File "/usr/local/lib/python3.6/site-packages/pyhap/hap_server.py", line 202, in dispatch
    getattr(self, self.HANDLERS[self.command][path])()
  File "/usr/local/lib/python3.6/site-packages/pyhap/hap_server.py", line 517, in handle_set_characteristics
    self.client_address)
  File "/usr/local/lib/python3.6/site-packages/pyhap/accessory_driver.py", line 443, in set_characteristics
    char.set_value(cq["value"], should_notify=False)
  File "/usr/local/lib/python3.6/site-packages/pyhap/characteristic.py", line 153, in set_value
    self.setter_callback(value)
  File "/usr/src/app/homeassistant/components/homekit/type_covers.py", line 53, in move_cover
    if value > self.current_position:
TypeError: '>' not supported between instances of 'int' and 'NoneType'

Additional information:

This is more of a feature request, but it would be good to in the future support covers which do no have set_cover_position service (i.e. support those with just open_cover, close_cover, and stop_cover)

[cdce8p]

Related #13656

Can you identify the cover that's causing the problem and upload a screenshot of the state (in the state panel)?

[nickw444]

Thanks for the fix. Sorry it seems you were faster at fixing it than I was at giving you information.

for the record, my cover config looks like this:

- platform: mqtt
  name: blindkit_nick_front
  command_topic: /things/blindkit/18fe3499d274/send_code
  availability_topic: /things/blindkit/18fe3499d274/status
  qos: 1
  payload_open: 100,0,OPEN
  payload_close: 100,0,CLOSE
  payload_stop: 100,0,STOP
  set_position_topic: /things/blindkit/18fe3499d274/send_code
  set_position_template: |-
    {% if position > 70 %}100,0,OPEN
    {% elif position < 30 %}100,0,CLOSE
    {% else %}100,0,STOP
    {% endif %}

source

Unfortunately my VPN/connection at home is down so I can't access my HomeAssistant instance at home at the moment. Let me know if you need any more info.

---

Additionally, as I mentioned in the issue, I think it would be useful to support covers which do no have set_cover_position service (i.e. support those with just open_cover, close_cover, and stop_cover) – perhaps by doing something like i've done with the set_position_template in my config. Should I open a separate issue for this? I'd be interested in implementing it myself (I started last night, but couldn't get far because of this bug).

[cdce8p]

Unfortunately I'm not sure if it's really a fix or more a limit to only accept certain values. If you can, check out the current dev branch and see if it works or doesn't, but doesn't throw any errors.

---

If you want to implement it yourself, feel free to open a WIP PR. I'm happy to help if you have questions.

[nickw444]

@cdce8p looks like this is still an issue:

Exception happened during processing of request from ('192.168.20.197', 49204)
Traceback (most recent call last):
  File "/usr/local/Cellar/python/3.6.4_4/Frameworks/Python.framework/Versions/3.6/lib/python3.6/socketserver.py", line 639, in process_request_thread
    self.finish_request(request, client_address)
  File "/Users/nickw/.local/share/virtualenvs/home-assistant-bB22INbG/lib/python3.6/site-packages/pyhap/hap_server.py", line 805, in finish_request
    self.RequestHandlerClass(sock, client_addr, self, self.accessory_handler)
  File "/Users/nickw/.local/share/virtualenvs/home-assistant-bB22INbG/lib/python3.6/site-packages/pyhap/hap_server.py", line 141, in __init__
    super(HAPServerHandler, self).__init__(sock, client_addr, server)
  File "/usr/local/Cellar/python/3.6.4_4/Frameworks/Python.framework/Versions/3.6/lib/python3.6/socketserver.py", line 696, in __init__
    self.handle()
  File "/usr/local/Cellar/python/3.6.4_4/Frameworks/Python.framework/Versions/3.6/lib/python3.6/http/server.py", line 420, in handle
    self.handle_one_request()
  File "/usr/local/Cellar/python/3.6.4_4/Frameworks/Python.framework/Versions/3.6/lib/python3.6/http/server.py", line 406, in handle_one_request
    method()
  File "/Users/nickw/.local/share/virtualenvs/home-assistant-bB22INbG/lib/python3.6/site-packages/pyhap/hap_server.py", line 202, in dispatch
    getattr(self, self.HANDLERS[self.command][path])()
  File "/Users/nickw/.local/share/virtualenvs/home-assistant-bB22INbG/lib/python3.6/site-packages/pyhap/hap_server.py", line 517, in handle_set_characteristics
    self.client_address)
  File "/Users/nickw/.local/share/virtualenvs/home-assistant-bB22INbG/lib/python3.6/site-packages/pyhap/accessory_driver.py", line 443, in set_characteristics
    char.client_update_value(cq["value"])
  File "/Users/nickw/.local/share/virtualenvs/home-assistant-bB22INbG/lib/python3.6/site-packages/pyhap/characteristic.py", line 176, in client_update_value
    self.setter_callback(value)
  File "/Users/nickw/Documents/personal/home-assistant/homeassistant/components/homekit/type_covers.py", line 52, in move_cover
    if value > self.current_position:
TypeError: '>' not supported between instances of 'int' and 'NoneType'

Config:

cover:
  - platform: mqtt
    name: blindkit_nick_front
    command_topic: /things/blindkit/18fe3499d274/send_code
    availability_topic: /things/blindkit/18fe3499d274/status
    qos: 1
    payload_open: 100,0,OPEN
    payload_close: 100,0,CLOSE
    payload_stop: 100,0,STOP
    set_position_topic: /things/blindkit/18fe3499d274/send_code
    set_position_template: |-
      {% if position > 70 %}100,0,OPEN
      {% elif position < 30 %}100,0,CLOSE
      {% else %}100,0,STOP
      {% endif %}
  - platform: mqtt
    name: blindkit_nick_side
    command_topic: /things/blindkit/18fe3499d274/send_code
    availability_topic: /things/blindkit/18fe3499d274/status
    qos: 1
    payload_open: 100,1,OPEN
    payload_close: 100,1,CLOSE
    payload_stop: 100,1,STOP
    set_position_topic: /things/blindkit/18fe3499d274/send_code
    set_position_template: |-
      {% if position > 70 %}100,0,OPEN
      {% elif position < 30 %}100,0,CLOSE
      {% else %}100,0,STOP
      {% endif %}

Running current dev branch: 8d48164f25f3b7f272ee486ecdeb6e1e8e4c6174

[nickw444]

This is the initial state from the state panel:

If I open/close them Home Assistant, I then get:

[cdce8p]

I should have read the error log more carefully. I fixed a different potential error :sweat_smile: Although I like your PR, it won't solve this (I will still review it, since it adds some good functionality). I think I know a fix, PR coming soon.

[nickw444]

I was able to fix the error by setting the initial value for self.current_position in the constructor to 0. (But of course this doesn't make the cover work in my situation, since the cover does not report a position value, rather just a state value of open/closed.

[cdce8p]

@nickw444 I had a look at your configuration again. It seems like your covers don't really support set_cover_position, do they? At least they don't seem to report the current_position parameter which is way you're noticing this issue. Have you tried it without: set_position_topic and set_position_template? I haven't worked with MQTT yet, maybe there is a way to set current_position to a valid value.

[nickw444]

Have you tried it without: set_position_topic and set_position_template?

Sure have, see my comment above: https://github.com/home-assistant/home-assistant/issues/13676#issuecomment-378778634

I think even though they set_cover_position when they are configured like this, it does not work because HA has no way to report the current position, and simply knows whether they are open/closed.

maybe there is a way to set current_position to a valid value.

Maybe there is, but perhaps it would be better to simply support covers which do not expose these state values? See my PR #13819 for a basic/rough implementation to get this working.

[cdce8p]

The issue is that even if your PR is merge, which I support, your cover will still be added as WindowCovering since the supported_features parameter says it supports set_cover_position although it doesn't.

[nickw444]

Yep you’re right about that! But if my PR was approved I would be able to remove my ‘hack’ from my config and simply use the open/close cover services and no longer provide the set cover positon service traits.

[cdce8p]

Year, but then this isn't an issue. If you try to use an unsupported component, you shouldn't expect it to work. I will close this then.

Your PR should be ready for 0.67 if nothing comes in between. We can continue there.

[nickw444]

If you try to use an unsupported component, you shouldn't expect it to work. I will close this then.

Well it technically is supported. Maybe the docs should be updated to reflect the fact the cover must support the ATTR_CURRENT_POSITION state value too?

https://github.com/home-assistant/home-assistant/blob/0daf38d18c2171455865ca4d3fc7e14fb3449d62/homeassistant/components/homekit/type_covers.py#L92

Currently the docs only describe it as requiring the set_cover_position service, which happens to be true for MQTT covers which implement the config for set_position_template

Perhaps the docs should say something like:

All covers that support set_cover_position and have the current_position state attribute.

And to be even more safe, perhaps an additional check here so that covers which don't have the attribute can't be used for homekit:

https://github.com/home-assistant/home-assistant/blob/0daf38d18c2171455865ca4d3fc7e14fb3449d62/homeassistant/components/homekit/__init__.py#L102-L103

---

On a side note, I like the refactor you've done in __init__.py to tidy up the accessories! Nice!

[cdce8p]

Technically you're right. However for set_cover_position to work correctly, the cover must have a valid current_position. That's what the docs meant there. Adding additional checks isn't really practical since we would need to check if the current_position is an integer. That way we would miss out on any special cases where the cover is first unavailable and only gets a valid state later (including a valid `current_position). The feature flags are there to indicate if something is supported. We should continue to rely on those.