home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
69.73k stars 28.88k forks source link

Invalid Lennox E30 pairing flow via HomeKit controller #20885

Closed jeromelaban closed 4 years ago

jeromelaban commented 5 years ago

Home Assistant release with the issue:

0.87.0

Last working Home Assistant release (if known): None

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

Component/platform:

Homekit controller

Description of problem: Here's the "normal" flow for pairing a Lennox E30 to home kit:

Pairing a Lennox E30 with home-assistant using the HomeKit Controler does not work properly, for two reasons:

Tracking down the issue in homekit_python, I could create a manual pairing by adding:

    pin = input("PIN: ")

at this line: https://github.com/jlusiardi/homekit_python/blob/9604d76131a9955c8b4dbd9f10dc92394cc61ae3/homekit/protocol/__init__.py#L102

and run it as CLI:

python3 pair.py -d XX:XX:XX:XX:XX:XX -f pairing.json -a "LennoxE30" -p 123456

Blocking the pairing procedure to input the pin at the precise moment (after step 3, the verify request) during the negotiation makes the pairing code appear and stay visible long enough on the Lennox E30 screen to be able to input it at the CLI.

I've yet to be able to reuse the pairing file in home-assistant, and I'm not sure either how to create such an asynchronous flow using the HA UI.

Update 2019-02-09: I was able to reuse in HA the pairing I made using my modified CLI of homekit_python. The name of the pairing has to be the device ID XX:XX:XX:XX:XX:XX (AccessoryPairingID value in the pairing file). I'm only able to see the temperature, modes are not available and setting the temperature does not seem to have any actual effect.

garyak commented 4 years ago

iOS 13.1 on iPhone 8. HA running on a Hass.io install on Intel NUC under Ubuntu 18.04. S30 firmware is version 03.50.0148 updated August 28, 2019. Using the built-in HA Thermostat card. These are the states reported by HA:

`climate.icomfort_s30_02f6a2 off hvac_modes: off,heat,cool,heat_cool current_temperature: 76 min_temp: 40 max_temp: 90 temperature: 58 current_humidity: 38 hvac_action: off friendly_name: Master Thermostat supported_features: 1`
climate.icomfort_s30_22eed8 off hvac_modes: off,heat,cool,heat_cool current_temperature: 78 min_temp: 40 max_temp: 90 temperature: 65 current_humidity: 27 hvac_action: off friendly_name: Kitchen Thermostat supported_features: 1

I haven't restarted since the S30's paired a couple days ago. I have loging set to "ERROR". No errors are appearing for this integration.

-- | -- | --  

Jc2k commented 4 years ago

@garyak nice. it sounds like you had the same problem as @GaryOkie originally, which definitely makes me think its a race as nothing in that bit of code has changed in a while.

You updated your firmware on August 28 - that was after you last posted here. Can you confirm you experienced the error again after that update? Specificially the JSONDecodeError? If not then maybe the latest firmware helped.

Where there any other changes - like in your networking etc between the last time you noticed and error and it working?

Did it just work first time with 99.3 or did you try more than once? What version were you on previously?

garyak commented 4 years ago

Chamberlin pushes updates to the S30. It's likely @GaryOkie is at the same version, but if not, has a means to verify the S30 part. I have not received a JSONDecodeError in the HA logs.

No changes to networking. S30's are on the same VLAN as the HA instance. S30's are WiFi @ 2.4Ghz, HA is hardwired.

Pairing did not work first time. I fumbled the first pairing code by not entering the dashes. The result was HA throwing a Pairing attempt failed with an unhandled exception error. I restarted WAC, without a complete reset, and HA rediscovered the S30. The second S30 integrated first time. I have not exported the S30's from HA to HomeKit.

Previous version was 0.94.xx. I added zeroconf: to the config file and made several attempts at pairing. S30 firmware was updated after those attempts. What prompted the new attempt was that I found both S30 in WAC mode. Believing there had been a firmware update, I retried the pairing procedure. Not sure why the S30's went into WAC mode. No indication of power transient.

skyway007 commented 4 years ago

Wow, nice to see that you got it to work @garyak I'm on the same new version as well and have tried on every new version of HA with the same results. The S30 thinks it paired okay but HA doesn't. I will try adding zeconf and try again tonight. Network is relatively basic and on the same VLAN. Will report back. Thanks for your help @Jc2k. Would it be valuable to go ahead and add the retry to see if that helps us?

GaryOkie commented 4 years ago

@garyak - According to this site, the latest iComfort S30 software update was v3.5 in May 2019, which is automatically installed. (By Lennox, not Chamberlain - BTW! :)

That interesting that your S30's went into WAC mode on their own, but something surely caused them to reboot. I'm sure you already know there is an option in the S30 Homekit menu of "Auto WAC mode" that is enabled by default. It is triggered if the S30 reboots and WAC was not previously configured.

It's also interesting that the HA zeroconf detection picked up the Homekit broadcast while the S30 was only in the initial WAC "ready for configuration" mode. I didn't think the broadcast occurred until after the WAC wifi connection was completed via iOS (after tapping Next then done in iOS).

@skyway007 - would you be able to work with @jc2k to implement the additional debugging tests he is needing? I won't be able to for another week or two. If you are using HASSIO, I can help you with how to update the python code.

Jc2k commented 4 years ago

+1 - I need to understand what the failure is before I can add appropriate error handling and retrying. The 20s sleep not working is somewhat troubling.

My understanding was that HomeKit devices should always broadcast. At least they need to when paired for networks where IPs are dynamic (most non techie DHCP setups). So the whole WAC thing is confusing. Unless a paired S30 does always broadcast?

skyway007 commented 4 years ago

@GaryOkie yes absolutely. I'm definitely willing. Honestly the challenge has been getting to a point where I can access the core files. I use a container within docker so have to setup SSH to it I believe. I will work on it today and then work on making the changes. Any feedback to update the python would be appreciated of course. Will report back tonight.

GaryOkie commented 4 years ago

Hi @skyway007 - that's great you can pick up with the additional debugging! Happy to help.

I can only speak to a HASSIO configuration, but it sounds like your docker configuration is similar. I use the community SSH & web Terminal add-on, which requires disabling "protection mode" then restarting HA.

Once connected to HASSIO via SSH terminal, I can access the the HA Docker container via this command: docker exec -it homeassistant /bin/bash

Then you can edit (I use vi) the following python code: /usr/local/lib/python3.7/site-packages/homekit/controller/ip_implementation.py

Then insert the THREE new "logging.warning" debug statements as per below...

def list_accessories_and_characteristics(self):
        """
        This retrieves a current set of accessories and characteristics behind this pairing.
        :return: the accessory data as described in the spec on page 73 and following
        :raises AccessoryNotFoundError: if the device can not be found via zeroconf
        """
        logging.warning("LIST_accessories_and_characteristics")
        import time; time.sleep(10)
        if not self.session:
            logging.warning("STARTING NEW SESSION")
            self.session = IpSession(self.pairing_data)
        try:
            response = self.session.get('/accessories')
        except (AccessoryDisconnectedError, EncryptionError):
            self.session.close()
            self.session = None
            raise
        tmp = response.read().decode()
        logging.warning("GOT RESPONSE %s", tmp)
        accessories = json.loads(tmp)['accessories']

Be sure you have at least WARN level logging enabled in configuration.yaml, then restart HA. Reset the S30 Homekit to factory defaults, which should then automatically enter WAC mode and you then set up the wifi connection with iOS. HA (via zeroconf) should automatically detect the S30 in the notification panel to start the pairing process. Capture the relevant messages from the home-assistant.log for @jc2k .

garyak commented 4 years ago

@GaryOkie got MyQ on my mind. I hope my credibility isn't too damaged. I verified both S30's are on 03.50.0148 updated August 28, 2019 according the S30's About screen. As for WAC mode triggering, I sure you're correct. I had reset both S30's after pounding them testing last time. Must have had a minor power drop. I do not have them configured in HomeKit.

Yes, you need to wait for the S30 to configure the SSID and press Done at the Find App screen to receive an HA code prompt. Sorry for the confusion. I'll increase my dose to two cups tomorrow.

garyak commented 4 years ago

@Jc2k I have DHCP reservations for both S30's. If it's relevant.

skyway007 commented 4 years ago

@GaryOkie thank you very much for taking the time to spell this out. I believe I was able to make all the right changes and I'm ready to go. Will try it with the S30 tonight when I get home.

skyway007 commented 4 years ago

Hi @Jc2k, here's what I got.

I'm using HA with Docker v0.99.2. S30 is on same VLAN as HA.

HA doesn't have any trouble recognizing it as a new Home Kit device ready to connect. When I attempt to configure it, the S30 promptly displays the code and after I enter the code into HA, the S30 successfully confirms that it is paired. HA continues to spin and then ultimately gives me the error. The flow, error and timing has been largely unchanged for me in the last few version of HA.

Please let me know if you need anything else. Thank you!

2019-09-30 19:46:44 WARNING (SyncWorker_15) [root] LIST_accessories_and_characteristics
2019-09-30 19:46:54 WARNING (SyncWorker_15) [root] STARTING NEW SESSION
2019-09-30 19:47:04 WARNING (SyncWorker_15) [root] GOT RESPONSE 
2019-09-30 19:47:04 ERROR (MainThread) [homeassistant.components.homekit_controller.config_flow] Pairing attempt failed with an unhandled exception
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/components/homekit_controller/config_flow.py", line 251, in async_step_pair
    return await self._entry_from_accessory(pairing)
  File "/usr/src/homeassistant/homeassistant/components/homekit_controller/config_flow.py", line 324, in _entry_from_accessory
    pairing.list_accessories_and_characteristics
  File "/usr/local/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/usr/local/lib/python3.7/site-packages/homekit/controller/ip_implementation.py", line 85, in list_accessories_and_characteristics
    accessories = json.loads(tmp)['accessories']
  File "/usr/local/lib/python3.7/json/__init__.py", line 348, in loads
    return _default_decoder.decode(s)
  File "/usr/local/lib/python3.7/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/local/lib/python3.7/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
2019-09-30 19:47:04 ERROR (MainThread) [homeassistant.components.homekit_controller.config_flow] Pairing attempt failed with an unhandled exception
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/components/homekit_controller/config_flow.py", line 278, in async_step_pair
    start_pairing, self.hkid, self.hkid
  File "/usr/local/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/usr/local/lib/python3.7/site-packages/homekit/controller/controller.py", line 374, in start_pairing
    raise AlreadyPairedError('Alias "{a}" is already paired.'.format(a=alias))
homekit.exceptions.AlreadyPairedError: Alias "4A:A4:6E:73:E1:60" is already paired.
GaryOkie commented 4 years ago

Thanks @skyway007 for implementing this test! It's an identical result to my test, but with confirmation that a NEW SESSION is being started that @jc2k wanted to see.

As you can see, there is no accessory data in the log following GOT RESPONSE (tmp) - just python errors primarily due to tmp being null. Seems reasonable to test for a null tmp and exit with a related message. But of course, getting to the bottom of why nothing is returned and solving it, that data verification shouldn't be needed.

While we are waiting for @jc2k to suggest additional debugging code to try, please change time.sleep(10) to time.sleep(30) to see if that makes any difference.

EDIT: Also, please see if your Lennox S30 is on the latest firmware release that @garyak identified on his units... 03.50.0148 according the S30's About screen.

Jc2k commented 4 years ago

Thanks @GaryOkie - thats exactly right - i want to keep bumping the time.sleep to see if there is a value that works.

I've verified that the existing code closes the TCP stream used for pairing (i had worried that the stream was getting left open and that was to blame, but this is now ruled out).

Another thought was that the listen port was changing and without an updated Bonjour search result this wasn't been detected. But that would manifest as a ConnectionTimeout, not an empty string being returned.

I guess that is one thing to look at actually - although this value is empty, there should have been a full HTTP response with headers. So we can do something like this. Find homekit/http_impl/response.py. It should look like this. Find the parse function and edit as follows:

    def parse(self, part):
        import logging
        logging.warning(part)
        self._raw_response += part
        pos = self._raw_response.find(b'\r\n')
        while pos != -1:

(This module doesn't have logging already hence us adding the import).

This well let us see the full decrypted response from the device which might include a HTTP error code.

So that's 3 more pieces of info:

skyway007 commented 4 years ago

Hi @Jc2k so my unit is definitely on 3.5.0.0148 and changing the value to 30 did the trick!!!!

I also added the other logging.

It took a long time. And from the logs it almost looks like it had to do it a couple of times but it finally configured it okay. HA is reporting the state and I'm able to control based on very preliminary testing.

Here's the logs.

2019-10-02 16:58:39 WARNING (SyncWorker_12) [root] LIST_accessories_and_characteristics
2019-10-02 16:59:10 WARNING (SyncWorker_12) [root] STARTING NEW SESSION
2019-10-02 16:59:10 WARNING (SyncWorker_12) [root] bytearray(b'HTTP/1.1 200 OK\r\nContent-Type: application/hap+json\r\nContent-Length: 1853\r\n\r\n{"accessories":[{"aid":1,"services":[{"characteristics":[{"format":"bool","iid":2,"perms":["pw"],"type":"14"},{"format":"string","iid":3,"perms":["pr"],"type":"20","value":"Lennox"},{"format":"string","iid":4,"perms":["pr"],"type":"21","value":"S30 2B"},{"format":"string","iid":5,"perms":["pr"],"type":"23","value":"iComfort S30 3eaf73"},{"format":"string","iid":6,"perms":["pr"],"type":"30","value":"WL17J00671"},{"format":"string","iid":7,"perms":["pr"],"type":"52","value":"3.55.626"},{"format":"string","iid":8,"perms":["pr"],"type":"53","value":"3.0.002"}],"iid":1,"type":"3E"},{"characteristics":[{"format":"uint8","iid":101,"maxValue":2,"minStep":1,"minValue":0,"perms":["pr","ev"],"type":"F","value":0},{"format":"uint8","iid":102,"maxValue":3,"minStep":1,"minValue":0,"perms":["pr","pw","ev"],"type":"33","value":0},{"format":"float","iid":103,"maxValue":100,"minStep":0.1,"minValue":0,"perms":["pr","ev"],"type":"11","unit":"celsius","value":25.5},{"format":"float","iid":104,"maxValue":32,"minStep":0.5,"minValue":4.5,"perms":["pr","pw","ev"],"type":"35","unit":"celsius","value":21},{"format":"uint8","iid":105,"maxValue":1,"minStep":1,"minValue":0,"perms":["pr","pw","ev"],"type":"36","value":1},{"format":"float","iid":106,"maxValue":37,"minStep":0.5,"minValue":15.5,"perms":["pr","pw","ev"],"type":"D","unit":"celsius","value":23},{"format":"float","iid":107,"maxValue":100,"minStep":1,"minValue":0,"perms":["pr","ev"],"type":"10","unit":"percentage","value":56},{"format":"float","iid":108,"maxValue":32,"minStep":0.5,"minValue":4.5,"perms":["pr","pw","ev"],"type":"12","unit":"celsius","value":21},{"format":"string","iid":109,"perms":["pr"],"type":"23","value":"Thermostat"}],"iid":100,"primary":true,"type":"4A"},{"characteristics":[{"format":"string","iid":301,"perms":["pr"],"type":"37","value":"1.1.0"}],"iid":300,"type":"A2"}]}]}')
2019-10-02 16:59:10 WARNING (SyncWorker_12) [root] GOT RESPONSE {"accessories":[{"aid":1,"services":[{"characteristics":[{"format":"bool","iid":2,"perms":["pw"],"type":"14"},{"format":"string","iid":3,"perms":["pr"],"type":"20","value":"Lennox"},{"format":"string","iid":4,"perms":["pr"],"type":"21","value":"S30 2B"},{"format":"string","iid":5,"perms":["pr"],"type":"23","value":"iComfort S30 3eaf73"},{"format":"string","iid":6,"perms":["pr"],"type":"30","value":"WL17J00671"},{"format":"string","iid":7,"perms":["pr"],"type":"52","value":"3.55.626"},{"format":"string","iid":8,"perms":["pr"],"type":"53","value":"3.0.002"}],"iid":1,"type":"3E"},{"characteristics":[{"format":"uint8","iid":101,"maxValue":2,"minStep":1,"minValue":0,"perms":["pr","ev"],"type":"F","value":0},{"format":"uint8","iid":102,"maxValue":3,"minStep":1,"minValue":0,"perms":["pr","pw","ev"],"type":"33","value":0},{"format":"float","iid":103,"maxValue":100,"minStep":0.1,"minValue":0,"perms":["pr","ev"],"type":"11","unit":"celsius","value":25.5},{"format":"float","iid":104,"maxValue":32,"minStep":0.5,"minValue":4.5,"perms":["pr","pw","ev"],"type":"35","unit":"celsius","value":21},{"format":"uint8","iid":105,"maxValue":1,"minStep":1,"minValue":0,"perms":["pr","pw","ev"],"type":"36","value":1},{"format":"float","iid":106,"maxValue":37,"minStep":0.5,"minValue":15.5,"perms":["pr","pw","ev"],"type":"D","unit":"celsius","value":23},{"format":"float","iid":107,"maxValue":100,"minStep":1,"minValue":0,"perms":["pr","ev"],"type":"10","unit":"percentage","value":56},{"format":"float","iid":108,"maxValue":32,"minStep":0.5,"minValue":4.5,"perms":["pr","pw","ev"],"type":"12","unit":"celsius","value":21},{"format":"string","iid":109,"perms":["pr"],"type":"23","value":"Thermostat"}],"iid":100,"primary":true,"type":"4A"},{"characteristics":[{"format":"string","iid":301,"perms":["pr"],"type":"37","value":"1.1.0"}],"iid":300,"type":"A2"}]}]}
2019-10-02 16:59:10 INFO (MainThread) [homeassistant.setup] Setting up homekit_controller
2019-10-02 16:59:10 INFO (MainThread) [homeassistant.setup] Setup of domain homekit_controller took 0.0 seconds.
2019-10-02 16:59:10 WARNING (SyncWorker_8) [root] LIST_accessories_and_characteristics
2019-10-02 16:59:40 WARNING (SyncWorker_8) [root] STARTING NEW SESSION
2019-10-02 16:59:40 WARNING (SyncWorker_8) [root] bytearray(b'HTTP/1.1 200 OK\r\nContent-Type: application/hap+json\r\nContent-Length: 1853\r\n\r\n{"accessories":[{"aid":1,"services":[{"characteristics":[{"format":"bool","iid":2,"perms":["pw"],"type":"14"},{"format":"string","iid":3,"perms":["pr"],"type":"20","value":"Lennox"},{"format":"string","iid":4,"perms":["pr"],"type":"21","value":"S30 2B"},{"format":"string","iid":5,"perms":["pr"],"type":"23","value":"iComfort S30 3eaf73"},{"format":"string","iid":6,"perms":["pr"],"type":"30","value":"WL17J00671"},{"format":"string","iid":7,"perms":["pr"],"type":"52","value":"3.55.626"},{"format":"string","iid":8,"perms":["pr"],"type":"53","value":"3.0.002"}],"iid":1,"type":"3E"},{"characteristics":[{"format":"uint8","iid":101,"maxValue":2,"minStep":1,"minValue":0,"perms":["pr","ev"],"type":"F","value":0},{"format":"uint8","iid":102,"maxValue":3,"minStep":1,"minValue":0,"perms":["pr","pw","ev"],"type":"33","value":0},{"format":"float","iid":103,"maxValue":100,"minStep":0.1,"minValue":0,"perms":["pr","ev"],"type":"11","unit":"celsius","value":25.5},{"format":"float","iid":104,"maxValue":32,"minStep":0.5,"minValue":4.5,"perms":["pr","pw","ev"],"type":"35","unit":"celsius","value":21},{"format":"uint8","iid":105,"maxValue":1,"minStep":1,"minValue":0,"perms":["pr","pw","ev"],"type":"36","value":1},{"format":"float","iid":106,"maxValue":37,"minStep":0.5,"minValue":15.5,"perms":["pr","pw","ev"],"type":"D","unit":"celsius","value":23},{"format":"float","iid":107,"maxValue":100,"minStep":1,"minValue":0,"perms":["pr","ev"],"type":"10","unit":"percentage","value":56},{"format":"float","iid":108,"maxValue":32,"minStep":0.5,"minValue":4.5,"perms":["pr","pw","ev"],"type":"12","unit":"celsius","value":21},{"format":"string","iid":109,"perms":["pr"],"type":"23","value":"Thermostat"}],"iid":100,"primary":true,"type":"4A"},{"characteristics":[{"format":"string","iid":301,"perms":["pr"],"type":"37","value":"1.1.0"}],"iid":300,"type":"A2"}]}]}')
2019-10-02 16:59:40 WARNING (SyncWorker_8) [root] GOT RESPONSE {"accessories":[{"aid":1,"services":[{"characteristics":[{"format":"bool","iid":2,"perms":["pw"],"type":"14"},{"format":"string","iid":3,"perms":["pr"],"type":"20","value":"Lennox"},{"format":"string","iid":4,"perms":["pr"],"type":"21","value":"S30 2B"},{"format":"string","iid":5,"perms":["pr"],"type":"23","value":"iComfort S30 3eaf73"},{"format":"string","iid":6,"perms":["pr"],"type":"30","value":"WL17J00671"},{"format":"string","iid":7,"perms":["pr"],"type":"52","value":"3.55.626"},{"format":"string","iid":8,"perms":["pr"],"type":"53","value":"3.0.002"}],"iid":1,"type":"3E"},{"characteristics":[{"format":"uint8","iid":101,"maxValue":2,"minStep":1,"minValue":0,"perms":["pr","ev"],"type":"F","value":0},{"format":"uint8","iid":102,"maxValue":3,"minStep":1,"minValue":0,"perms":["pr","pw","ev"],"type":"33","value":0},{"format":"float","iid":103,"maxValue":100,"minStep":0.1,"minValue":0,"perms":["pr","ev"],"type":"11","unit":"celsius","value":25.5},{"format":"float","iid":104,"maxValue":32,"minStep":0.5,"minValue":4.5,"perms":["pr","pw","ev"],"type":"35","unit":"celsius","value":21},{"format":"uint8","iid":105,"maxValue":1,"minStep":1,"minValue":0,"perms":["pr","pw","ev"],"type":"36","value":1},{"format":"float","iid":106,"maxValue":37,"minStep":0.5,"minValue":15.5,"perms":["pr","pw","ev"],"type":"D","unit":"celsius","value":23},{"format":"float","iid":107,"maxValue":100,"minStep":1,"minValue":0,"perms":["pr","ev"],"type":"10","unit":"percentage","value":56},{"format":"float","iid":108,"maxValue":32,"minStep":0.5,"minValue":4.5,"perms":["pr","pw","ev"],"type":"12","unit":"celsius","value":21},{"format":"string","iid":109,"perms":["pr"],"type":"23","value":"Thermostat"}],"iid":100,"primary":true,"type":"4A"},{"characteristics":[{"format":"string","iid":301,"perms":["pr"],"type":"37","value":"1.1.0"}],"iid":300,"type":"A2"}]}]}
2019-10-02 16:59:40 INFO (MainThread) [homeassistant.components.climate] Setting up climate.homekit_controller
2019-10-02 16:59:40 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new climate.homekit_controller entity: climate.icomfort_s30_3eaf73
2019-10-02 16:59:40 ERROR (MainThread) [homeassistant.components.alexa.capabilities] climate.icomfort_s30_3eaf73 (<class 'homeassistant.core.State'>) has unsupported state value 'unknown'
2019-10-02 16:59:41 ERROR (MainThread) [homeassistant.core] Error doing job: Task exception was never retrieved
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/components/alexa/state_report.py", line 45, in async_entity_state_listener
    hass, smart_home_config, alexa_changed_entity
  File "/usr/src/homeassistant/homeassistant/components/alexa/state_report.py", line 70, in async_send_changereport_message
    properties = list(alexa_entity.serialize_properties())
  File "/usr/src/homeassistant/homeassistant/components/alexa/entities.py", line 181, in serialize_properties
    for prop in interface.serialize_properties():
  File "/usr/src/homeassistant/homeassistant/components/alexa/capabilities.py", line 106, in serialize_properties
    prop_value = self.get_property(prop_name)
  File "/usr/src/homeassistant/homeassistant/components/alexa/capabilities.py", line 589, in get_property
    raise UnsupportedProperty(name)
homeassistant.components.alexa.errors.UnsupportedProperty: thermostatMode
GaryOkie commented 4 years ago

That's sure good news @skyway007 - thanks! It looks like after pairing, the component needs to query the accessory detail a second time during a separate "Setting up climate.homekit.controller" process. So there was at least a 30 + 30 second timed.wait to complete the pairing and setup process.

@Jc2k - I hope this provides you with sufficient background to insert a 10-second retry loop. Thanks so much for all your great attention to this code!

ccormier commented 4 years ago

@skyway007 did you add any other code? I'm not getting the same results as you with a 30s sleep. I've also tried 60 and 90 seconds without success.

My S30 is on version 3.5.0.0148 as well.

2019-10-03 09:56:44 WARNING (SyncWorker_37) [root] list_accessories_and_characteristics 2019-10-03 09:56:44 WARNING (SyncWorker_37) [root] beginsleep 2019-10-03 09:57:14 WARNING (SyncWorker_37) [root] endsleep 2019-10-03 09:57:14 WARNING (SyncWorker_37) [root] STARTING NEW SESSION 2019-10-03 09:57:44 WARNING (SyncWorker_37) [root] got response

Jc2k commented 4 years ago

@ccormier can you try the other change I posted in https://github.com/home-assistant/home-assistant/issues/20885#issuecomment-537523780? It won’t make it work but might give clues as to the underlying fault.

ccormier commented 4 years ago

@Jc2k I actually do have that code there, but it doesn't look like it's getting triggered, I don't see the associated logline

Jc2k commented 4 years ago

Interesting. If that code isn’t hit then it shouldn’t get as far as it has. I wonder if there is another copy of the homekit package in your install somehow?

ccormier commented 4 years ago

If I understand the flow correctly (good chance i dont) it looks like the _read_response method in SecureHttp calls the parse function only if there is data . I put some debugging lines in secure_httpy.py -> SecureHttp -> _read_response

def _read_response(self, timeout=10):                                                                               
    # following the information from page 71 about HTTP Message splitting:                                          
    # The blocks start with 2 byte little endian defining the length of the encrypted data (max 1024 bytes)         
    # followed by 16 byte authTag                                                                                   
    tmp = bytearray()                                                                                               
    exp_len = 1024                                                                                                  
    response = HttpResponse()                                                                                       
    while not response.is_read_completely():                                                                        
        logging.warn('_read_response: loop begin')                                                                  
        # make sure we read all blocks but without blocking to long. Was introduced to support chunked transfer mode
        # from https://github.com/maximkulkin/esp-homekit                                                         
        self.sock.setblocking(0)                                                                                  

        no_data_remaining = (len(tmp) == 0)                                                                       

        # if there is no data use the long timeout so we don't miss anything, else since there is still data go on
        # much quicker.                                                                                           
        if no_data_remaining:                                                                                     
            used_timeout = timeout                                                                                
            logging.warn('_read_response: no data remaining')                                                     
        else:                                                                                                     
            used_timeout = 0.01                                                                                   
            logging.warn('_read_response: data remaining')                                                        
        data_ready = select.select([self.sock], [], [], used_timeout)[0]                                          

        # check if there is anything more to do                                                                   
        if not data_ready and no_data_remaining:                                                                  
            logging.warn('_read_response: no data breaking out')                                                  
            break                                                            

2019-10-03 10:45:28 WARNING (SyncWorker_1) [root] list_accessories_and_characteristics 2019-10-03 10:45:28 WARNING (SyncWorker_1) [root] beginsleep 2019-10-03 10:46:58 WARNING (SyncWorker_1) [root] endsleep 2019-10-03 10:46:58 WARNING (SyncWorker_1) [root] STARTING NEW SESSION 2019-10-03 10:46:58 DEBUG (SyncWorker_1) [root] init session removed some debug lines... 2019-10-03 10:46:58 DEBUG (SyncWorker_1) [root] session established 2019-10-03 10:46:58 DEBUG (SyncWorker_1) [root] handle request: b'GET /accessories HTTP/1.1\r\nHost: 10.6.6.229:7373\r\n\r\n' 2019-10-03 10:46:58 WARNING (SyncWorker_1) [root] _read_response: loop begin 2019-10-03 10:46:58 WARNING (SyncWorker_1) [root] _read_response: no data remaining 2019-10-03 10:47:28 WARNING (SyncWorker_1) [root] _read_response: no data breaking out 2019-10-03 10:47:28 WARNING (SyncWorker_1) [root] got response

Jc2k commented 4 years ago

Yeah it is quite a pain to follow. I think you are right. Looks like _read_response can return an empty HttpResponse in that case. So its weirder than I thought - had assumed the encrypted session was set up and a HTTP service was running but returning an error http status code with no body. Instead the encrypted session is up but its returning a completely empty string.

The retry loop i was going to try based off @ccormier's report was something like this change to _entry_from_accessory in homeassistant/components/homekit_controller/config_flow.py:

    async def _entry_from_accessory(self, pairing):
        """Return a config entry from an initialized bridge."""
        # The bulk of the pairing record is stored on the config entry.
        # A specific exception is the 'accessories' key. This is more
        # volatile. We do cache it, but not against the config entry.
        # So copy the pairing data and mutate the copy.
        pairing_data = pairing.pairing_data.copy()

        # Use the accessories data from the pairing operation if it is
        # available. Otherwise request a fresh copy from the API.
        # This removes the 'accessories' key from pairing_data at
        # the same time.
        accessories = pairing_data.pop("accessories", None)
        if not accessories:
            while not accessories:
                try:
                    accessories = await self.hass.async_add_executor_job(
                        pairing.list_accessories_and_characteristics
                    )
                except Exception:
                    _LOGGER.exception("Error whilst retrieving pairing metadata")
                    # Clean up the session that just failed
                    pairing.session.close()
                    pairing.session = None
                    # Retry in 5s
                    import asyncio
                    await asyncio.sleep(5)
... snip....

Would be interested if that helped anyone.

ccormier commented 4 years ago

I've removed the sleep and added the retry code. It loops indefinitely for me.

2019-10-03 14:03:07 WARNING (SyncWorker_2) [root] list_accessories_and_characteristics
2019-10-03 14:03:07 WARNING (SyncWorker_2) [root] STARTING NEW SESSION
2019-10-03 14:03:08 WARNING (SyncWorker_2) [root] _read_response: loop begin
2019-10-03 14:03:08 WARNING (SyncWorker_2) [root] _read_response: no data remaining
2019-10-03 14:03:38 WARNING (SyncWorker_2) [root] _read_response: no data breaking out
2019-10-03 14:03:38 WARNING (SyncWorker_2) [root] got response 
2019-10-03 14:03:38 ERROR (MainThread) [homeassistant.components.homekit_controller.config_flow] Error whilst retrieving pairing metadata
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/components/homekit_controller/config_flow.py", line 326, in _entry_from_accessory
    pairing.list_accessories_and_characteristics
  File "/usr/local/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/usr/local/lib/python3.7/site-packages/homekit/controller/ip_implementation.py", line 86, in list_accessories_and_characteristics
    accessories = json.loads(tmp)['accessories']
  File "/usr/local/lib/python3.7/json/__init__.py", line 348, in loads
    return _default_decoder.decode(s)
  File "/usr/local/lib/python3.7/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/local/lib/python3.7/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
2019-10-03 14:03:43 WARNING (SyncWorker_31) [root] list_accessories_and_characteristics
2019-10-03 14:03:43 WARNING (SyncWorker_31) [root] STARTING NEW SESSION
2019-10-03 14:03:43 WARNING (SyncWorker_31) [root] _read_response: loop begin
2019-10-03 14:03:43 WARNING (SyncWorker_31) [root] _read_response: no data remaining
2019-10-03 14:04:13 WARNING (SyncWorker_31) [root] _read_response: no data breaking out
2019-10-03 14:04:13 WARNING (SyncWorker_31) [root] got response 
2019-10-03 14:04:13 ERROR (MainThread) [homeassistant.components.homekit_controller.config_flow] Error whilst retrieving pairing metadata
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/components/homekit_controller/config_flow.py", line 326, in _entry_from_accessory
    pairing.list_accessories_and_characteristics
  File "/usr/local/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/usr/local/lib/python3.7/site-packages/homekit/controller/ip_implementation.py", line 86, in list_accessories_and_characteristics
    accessories = json.loads(tmp)['accessories']
  File "/usr/local/lib/python3.7/json/__init__.py", line 348, in loads
    return _default_decoder.decode(s)
  File "/usr/local/lib/python3.7/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/local/lib/python3.7/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

...snip...

2019-10-03 14:09:34 WARNING (SyncWorker_18) [root] list_accessories_and_characteristics
2019-10-03 14:09:34 WARNING (SyncWorker_18) [root] STARTING NEW SESSION
2019-10-03 14:09:34 WARNING (SyncWorker_18) [root] _read_response: loop begin
2019-10-03 14:09:34 WARNING (SyncWorker_18) [root] _read_response: no data remaining
2019-10-03 14:10:04 WARNING (SyncWorker_18) [root] _read_response: no data breaking out
2019-10-03 14:10:04 WARNING (SyncWorker_18) [root] got response 
2019-10-03 14:10:04 ERROR (MainThread) [homeassistant.components.homekit_controller.config_flow] Error whilst retrieving pairing metadata
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/components/homekit_controller/config_flow.py", line 326, in _entry_from_accessory
    pairing.list_accessories_and_characteristics
  File "/usr/local/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/usr/local/lib/python3.7/site-packages/homekit/controller/ip_implementation.py", line 86, in list_accessories_and_characteristics
    accessories = json.loads(tmp)['accessories']
  File "/usr/local/lib/python3.7/json/__init__.py", line 348, in loads
    return _default_decoder.decode(s)
  File "/usr/local/lib/python3.7/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/local/lib/python3.7/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
2019-10-03 14:10:09 WARNING (SyncWorker_16) [root] list_accessories_and_characteristics
2019-10-03 14:10:09 WARNING (SyncWorker_16) [root] STARTING NEW SESSION
2019-10-03 14:10:09 WARNING (SyncWorker_16) [root] _read_response: loop begin
2019-10-03 14:10:09 WARNING (SyncWorker_16) [root] _read_response: no data remaining
Jc2k commented 4 years ago

OK change of plan. Let's revert that and not fail hard at this stage of pairing. Make _entry_from_accessory like this:

    async def _entry_from_accessory(self, pairing):
        """Return a config entry from an initialized bridge."""
        # The bulk of the pairing record is stored on the config entry.
        # A specific exception is the 'accessories' key. This is more
        # volatile. We do cache it, but not against the config entry.
        # So copy the pairing data and mutate the copy.
        pairing_data = pairing.pairing_data.copy()

        name = f"{self.model} ({self.hkid})"

        # Use the accessories data from the pairing operation if it is
        # available. Otherwise request a fresh copy from the API.
        # This removes the 'accessories' key from pairing_data at
        # the same time.
        accessories = pairing_data.pop("accessories", None)
        if not accessories:
            try:
                accessories = await self.hass.async_add_executor_job(
                    pairing.list_accessories_and_characteristics
                )
            except Exception:
                 _LOGGER.exception("Failure whilst retrieving config entry metadata")

        if accessories:
            bridge_info = get_bridge_information(accessories)
            name = get_accessory_name(bridge_info)

        return self.async_create_entry(title=name, data=pairing_data)

The name of the config entry will now be less pretty, but the pairing should actually be saved. It likely still won't work but now we can try things like power cycling HomeAssistant and the thermostat. And we can inspect the config entry, too.

EDIT: Forgot a try/except in my haste

ccormier commented 4 years ago

Gave it a shot. It shows up as a "configured" integration now, but with no devices. When restarting HA i get the following in the logs.

2019-10-03 15:32:36 WARNING (SyncWorker_31) [root] list_accessories_and_characteristics
2019-10-03 15:32:36 WARNING (SyncWorker_31) [root] STARTING NEW SESSION
2019-10-03 15:32:36 WARNING (SyncWorker_31) [root] _read_response: loop begin
2019-10-03 15:32:36 WARNING (SyncWorker_31) [root] _read_response: no data remaining
2019-10-03 15:33:06 WARNING (SyncWorker_31) [root] _read_response: no data breaking out
2019-10-03 15:33:06 WARNING (SyncWorker_31) [root] got response 
2019-10-03 15:33:06 ERROR (MainThread) [homeassistant.config_entries] Error setting up entry S30 1B (A9:40:C7:55:C8:F8) for homekit_controller
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/config_entries.py", line 190, in async_setup
    hass, self
  File "/usr/src/homeassistant/homeassistant/components/homekit_controller/__init__.py", line 187, in async_setup_entry
    if not await conn.async_setup():
  File "/usr/src/homeassistant/homeassistant/components/homekit_controller/connection.py", line 129, in async_setup
    if await self.async_refresh_entity_map(self.config_num):
  File "/usr/src/homeassistant/homeassistant/components/homekit_controller/connection.py", line 177, in async_refresh_entity_map
    self.pairing.list_accessories_and_characteristics
  File "/usr/local/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/usr/local/lib/python3.7/site-packages/homekit/controller/ip_implementation.py", line 86, in list_accessories_and_characteristics
    accessories = json.loads(tmp)['accessories']
  File "/usr/local/lib/python3.7/json/__init__.py", line 348, in loads
    return _default_decoder.decode(s)
  File "/usr/local/lib/python3.7/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/local/lib/python3.7/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
2019-10-03 15:33:06 WARNING (SyncWorker_30) [root] list_accessories_and_characteristics
2019-10-03 15:33:06 WARNING (SyncWorker_30) [root] _read_response: loop begin
2019-10-03 15:33:06 WARNING (SyncWorker_30) [root] _read_response: no data remaining
2019-10-03 15:33:36 WARNING (SyncWorker_30) [root] _read_response: no data breaking out
2019-10-03 15:33:36 WARNING (SyncWorker_30) [root] got response 
2019-10-03 15:33:36 ERROR (MainThread) [homeassistant.core] Error doing job: Task exception was never retrieved
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/components/homekit_controller/connection.py", line 177, in async_refresh_entity_map
    self.pairing.list_accessories_and_characteristics
  File "/usr/local/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/usr/local/lib/python3.7/site-packages/homekit/controller/ip_implementation.py", line 86, in list_accessories_and_characteristics
    accessories = json.loads(tmp)['accessories']
  File "/usr/local/lib/python3.7/json/__init__.py", line 348, in loads
    return _default_decoder.decode(s)
  File "/usr/local/lib/python3.7/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/local/lib/python3.7/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
Jc2k commented 4 years ago

Are you able to restart the thermostat?

ccormier commented 4 years ago

I restarted, thermostat and thermostat hub, then HA. Same results in logs

GaryOkie commented 4 years ago

So the initial time.sleep before starting a new session is creating an exact delay as intended, but it obviously can't be counted on to ensure a reliable http response from an S30.

What about the separate timeout in SecureHttp ? def __init__(self, session, timeout=10)

This is used in the select.select module where timeout is a floating point value in seconds.

What's odd is that the logs show just a half-second differential between the session start/no data remaining, and when the loop breaks out. Clearly not 10.0 seconds. I'm not a Python programmer, but could there be a type/conversion issue here?

2019-10-03 15:33:06 WARNING (SyncWorker_30) [root] list_accessories_and_characteristics
2019-10-03 15:33:06 WARNING (SyncWorker_30) [root] _read_response: loop begin
2019-10-03 15:33:06 WARNING (SyncWorker_30) [root] _read_response: no data remaining
2019-10-03 15:33:36 WARNING (SyncWorker_30) [root] _read_response: no data breaking out
2019-10-03 15:33:36 WARNING (SyncWorker_30) [root] got response 
ccormier commented 4 years ago

Found the source of the issue. It turns out the pairing was fine, the problem is when attempting to retrieve the accessories list. There is a bug related to size of the request, I don't know if this is on the sending or receiving side.

By padding the session.get /accessories request in the homekit library I can successfully list accessories.

Here is the workaround, minimum pad was 6 characters (padding the user-agent because it seemed clean, but padding can be elsewhere): homekit/http_imp/secure_http.py

    def get(self, target):
        data = 'GET {tgt} HTTP/1.1\nUser-Agent: homekit-python/0.15.0\nHost: {host}:{port}\n\n'.format(tgt=target, host=self.host, port=self.port)
Jc2k commented 4 years ago

Yes, it did seem like the pair-setup had worked, a new TCP stream had been created and that pair-verify had set up a secure session. It seemed unlikely pair-verify would pass if pair-setup had generated duff crypto keys. It's great that you have been able to verify this.

In summary:

I'm at a loss as to how @ccormier case relates to @skyway007 and @garyak despite the obviously identical tracebacks so i feel like thats not the quite full story yet. Are their external factors at play? Magic external factors which might explain why what we think is happening isn't really?

These tiny changes in size could bit us if our multi-block crypto is broken. But (1) isn't enough to account for 6 bytes of padding that @ccormier needs, and (2) and (3) don't change the size of the request. Are there any others?

A problem we have with padding is that adding new headers is "off spec". The tado thermostat chokes on fairly normal (but not HAP) HTTP headers. So we only use headers we've seen the real iOS client use. So we need to see why a actual iOS client works here and we don't. I'm pretty sure the headers we send are the same as an actual iOS client (though it has been a while since i checked).

The big difference i know about is the Host header. The spec is vague about what Host /should/ contain but in practice where an actual iOS client sends Host: devicename._hap._tcp.local:1234 we send Host: 123.123.123.123:1234. If we were accurate here this would obviously get us an extra few bytes.

The real question is, is this papering over a real bug in the encryption code on our side? Or is this a bug on their side? It feels like if we couldn't successfully encrypt small requests we would have hit this problem with other devices?

ccormier commented 4 years ago

Good summary, I also believe that the different cases can be explained by environmental factors. Likely the IP addresses as you pointed out already. For example, the address of my "Host: " header is fairy short 10.6.6.229, whereas someone else is likely be using something like 192.168.100.200. Thats a different of 5 extra characters already, one short of my 6 required. That said it looks like most people would be right on the border of hitting that minimum magic size.

Re @skyway007 case: he did mention that he added some extra debugging lines, I'm curious if he also changed something in his headers or something to effect the size of the request. In my testing playing with timeouts alone wouldn't help.

My report of "6 characters" more is ambiguous, so here is some more data around it. The "extra" characters that I'm adding are the "123456" and "12345" in the "Host" header in the examples below.

Successful testcase:
python3 -m homekit.get_accessories -f ~/homekit -a s30
handle request: b'GET /accessories HTTP/1.1\r\nHost: 12345610.6.6.229:7373\r\n\r\n'
_handle_request len loop begin
unencrypted request data len: 58
encrypted request len: 76
encrypted request: b':\x00\x08\xf9v\xadmj1\xc9\xf6ZV/\x0c\x08\xa6\x1f\xb4\xea\xe96\xbf\t\xc4\xc2\x8a\x00\xcf\xa1\xc6|1\x0bF\x9c\x0f\x92d\x07\xfb\xb1\xc5\x81\xa9f\xa6Z$\t\x0b\xa5\xb9\x82\xa0j9\xd7su\x01\x0b\xe6\xab\x8c3l\xea\x0c\xf7Zln{av'
1.1: >accessory-information<
...snip...
Failed testcase
python3 -m homekit.get_accessories -f ~/homekit -a s30
handle request: b'GET /accessories HTTP/1.1\r\nHost: 1234510.6.6.229:7373\r\n\r\n'
_handle_request len loop begin
unencrypted request data len: 57
encrypted request len: 75
encrypted request: b'9\x00|\x8e\x8d\x88(\xa7\xcf\x86>\xa1\x1aa\x03(\x81\xcc\xb9T\xa2\x10Q\xd1\x81.\x06\xdd\x85 \xccE\xe7\xd4\xe4\x86\xaaDi\xe5\xcb\x07w\x11\xaf\xca\rU\x92\xdd\xeb\x14QBF\xc4\x98\x13\xe6\xd5\x01\xe6[\xe2\xf3\xa7j\x8c\x07]\xf1\x8d\xd9\xc5\xf1'
Expecting value: line 1 column 1 (char 0)

Re: multi crypto block I believe this is working. Normally it's only creating a single block. I've also tried creating multiple blocks by reducing the len_data = min(len(data), 32). This forces the code to split my data up into 2 blocks and it is successful without the padded request. Summing up the 2 blocks resulted in a total request of size 88.

python3 -m homekit.get_accessories -f ~/homekit -a s30
handle request: b'GET /accessories HTTP/1.1\r\nHost: 10.6.6.229:7373\r\n\r\n'
_handle_request len loop begin
unencrypted request data len: 52
encrypted request len: 50
encrypted request: b" \x002J\x88\xe6.\xb4z\x9b\xfc\xf7\xbb\xd3\x96\x14\xd2\x9b\xf1n\xafqX\x8d1){\x85\xa2B']aV\x9c\xd6\x9f\x8e\xb4\\\x07\xfc\xcd\xdd\x9fb\n\x97\xf2*"
_handle_request len loop begin
unencrypted request data len: 20
encrypted request len: 38
encrypted request: b'\x14\x00\x1eq\xb0O{\xd2\x17\x95\xcd\x9f\x00\xba\xe6\t!gD\x9e\xaeIC\xf4\xff=B\x7f.\x15\xb6`@\x19\x93\xd0Er'
1.1: >accessory-information<
...snip...

In summary, based on my testing I believe that the thermostat is waiting for a minimum amount of data to be received before processing the request. maybe it's waiting for a buffer or a highwater mark to get hit before processing the request (ie. minimum 76 bytes received). I think a valid workaround could be to make the Host header be more iOS like as you described. That should stay within the spec, be more iOS like and work around an issue such as this. As coincidence would have it, setting my hostname to "._hap._tcp.local:7373" results in a 76 byte request after encryption :)

ccormier commented 4 years ago

If someone else would like to verify the request size theory you can use the following code. homekit/http_impl/secure_http.py

    def _handle_request(self, data):
        logging.warn('handle request: %s', data)
        with self.lock:
            while len(data) > 0:
                logging.warn("_handle_request len loop begin")
                logging.warn("unencrypted request data len: %s", len(data))
                # split the data to max 1024 bytes (see page 71)
                len_data = min(len(data), 1024)
                tmp_data = data[:len_data]
                data = data[len_data:]
                len_bytes = len_data.to_bytes(2, byteorder='little')
                cnt_bytes = self.c2a_counter.to_bytes(8, byteorder='little')
                self.c2a_counter += 1
                ciper_and_mac = chacha20_aead_encrypt(len_bytes, self.c2a_key, cnt_bytes, bytes([0, 0, 0, 0]),
                                                      tmp_data)

                try:
                    request = len_bytes + ciper_and_mac[0] + ciper_and_mac[1]
                    logging.warn("encrypted request len: %s", len(request))
                    logging.warn("encrypted request: %s", request)
                    self.sock.send(request)
                except OSError as e:
                    raise exceptions.AccessoryDisconnectedError(str(e))

            return self._read_response(self.timeout)
GaryOkie commented 4 years ago

Count me in for testing this - but it will be at least another week before I can get onsite with the S30 to pair it. However, I can VPN to my remote site and run python test code, but I'm not sure how you set this get_accessories test case up.

I also can't tell what you changed (other than debug statements) in secure_http.py without dif'ing the original. I thought the most logical approach was to change the hostname to "._hap._tcp.local" to match the iOS client and also be a consistent 76 byte request?

Did you leave the original time.sleep(10) as-is, or do you recommend increasing it too?

FWIW, my S30 IP is 192.168.2.xxx (13 chars).

Jc2k commented 4 years ago

The proper fix is to set the host header the same way as an iOS client (based on the bonjour data). But the bonjour discovery data isn’t available from this part of the code and we don’t save it in the pairing data yet. So we need to save it in the pairing data and fall back to the current mechanism for existing pairings.

ccormier commented 4 years ago

@GaryOkie The code in https://github.com/home-assistant/home-assistant/issues/20885#issuecomment-538683053 will log some details around the request size. The work around would be to increase the size of your "request" by increasing the size the headers. The following is where you would manipulate the headers(junk prefixing the {host} worked in my case). No timeouts or debugging code is required. homekit/http_imp/secure_http.py

    def get(self, target):
        data = 'GET {tgt} HTTP/1.1\nHost: Xtrapadding123456X{host}:{port}\n\n'.format(tgt=target, host=self.host, port=self.port)
GaryOkie commented 4 years ago

Thanks @ccormier for the added context. The problem I face now is that I don't have a pairing file which is an essential component for running the get_accessories test. I understand that once HA successfully configures the Homekit accessory (which is not the case for me yet), the integration details are saved in the HA configuration.

Is it possible to manually create a pairing.json file from the following homekit.discover information (for testing purposes only)?

Name: iComfort S30 5185f0._hap._tcp.local.
Url: http_impl://192.168.1.23:7373
Configuration number (c#): 1
Feature Flags (ff): Supports HAP Pairing (Flag: 1)
Device ID (id): 4A:D4:7A:95:0A:42
Model Name (md): S30 2B
Protocol Version (pv): 1.0
State Number (s#): 1
Status Flags (sf): Accessory has been paired. (Flag: 0)
Category Identifier (ci): Thermostat (Id: 9)

EDIT: I did try: _python3 -m homekit.init_controllerstorage -f /config/pairing.json
which only created an empty pairing.json file, the best I could tell. Then I guessed at what might be a pairing file from the "demoserver.json" example in the documentation and created this:

{
  "name": " iComfort S30 5185f0._hap._tcp.local.",
  "host_ip": 192.168.1.23,
  "host_port": 7373,
  "accessory_pairing_id": "12:00:00:00:00:00",
  "accessory_pin": "123-45-678",
  "peers": {},
  "unsuccessful_tries": 0,
  "c#": 0,
  "category": "Thermostat"

}

Turns out this was not a good guess as the tests fail with "homekit.exceptions.ConfigLoadingError: Cannot parse "/config/pairing.json" as JSON file"

Jc2k commented 4 years ago

That’s the pairing file for an accessory not a controller.

You can’t create a pairing file manually - it’s generated as part of doing the pairing crypto dance. Even if the syntax was valid your device is unpaired and would not trust your fake crypto keys.

GaryOkie commented 4 years ago

ok, thanks @jc2k for clearing that up. So that counts me out as being able to run any further tests that require the pairing file.

Do you plan on providing an update that saves the iOS host header in the pairing data for examination? That's a test I can run when I get back to the remote site unless someone else is able to sooner.

Jc2k commented 4 years ago

I’m going to try but it might be hard to retrofit into the existing API, so might just fix it in the new async/push event client PR I’m working on instead.

GaryOkie commented 4 years ago

Hi @jc2k - I'm heading back to the S30 remote location now and will be able to try again later today with the HA pairing and any additional debugging or beta code you may want me to try. I've just now updated Hassio to .100.2 so I will be starting with the current production code unless I add back any logging (such as iOS host header capture).

I should have some time tomorrow for testing as well. Thanks!

GaryOkie commented 4 years ago

I went ahead and added the new logging @ccormier suggested to validate his request size theory, but there is a new and very significant variable that was just introduced by Lennox.

The S30 thermostat alerted me to a pending firmware update for Homekit (version 3.55.626). It was not automatically installed like other firmware updates have been. I have not found what this update involves, but I did notice that the S30 was now Bonjour broadcasting. Before the update, I had to initiate a Homekit factory reset and use an iOS device for WAC/wifi inclusion before it would broadcast and allow pairing.

Now, HA zeroconf immediately detects the S30 but since it was previously paired (unsuccessfully) to HA, it was ignored. I chose the S30 option to clear previous Homekit pairings instead of the full Homekit factory reset, so there was no need to enter WAC mode and use an iOS device in order to trigger pairing.

Once this incomplete pairing was cleared, the HA/zeroconf notification to configure the S30 appeared. Pairing was successful! Other than the logging statements, the only code change was the original timeout(10) @jc2k suggested. I did NOT add any padding to the header.

2019-10-15 10:41:27 WARNING (SyncWorker_4) [root] LIST_accessories_and_characteristics:  None                                                                                                                                            
2019-10-15 10:41:37 WARNING (SyncWorker_4) [root] STARTING NEW SESSION                                                                                                                                                                                                                                                                                                     
2019-10-15 10:41:37 WARNING (SyncWorker_4) [root] handle request: b'GET /accessories HTTP/1.1\r\nHost: 192.168.0.53:7373\r\n\r\n'                                                                                                        
2019-10-15 10:41:37 WARNING (SyncWorker_4) [root] _handle_request len loop begin                                                                                                                                                         
2019-10-15 10:41:37 WARNING (SyncWorker_4) [root] unencrypted request data len: 54                                                                                                                                                       
2019-10-15 10:41:37 WARNING (SyncWorker_4) [root] encrypted request len: 72                                                                                                                                                              
2019-10-15 10:41:37 WARNING (SyncWorker_4) [root] encrypted request: b'6\x00\x11\xed\<snip>'                                                                                                                                                                                                                                                                         
2019-10-15 10:41:37 WARNING (SyncWorker_4) [root] _read_response: loop begin...                                                                                                                                                          
2019-10-15 10:41:37 WARNING (SyncWorker_4) [root] _read_response: no data remaining, used_timeout=  10.0                                                                                                                                 
2019-10-15 10:41:37 WARNING (SyncWorker_4) [root] _read_response: loop begin...                                                                                                                                                          
2019-10-15 10:41:37 WARNING (SyncWorker_4) [root] _read_response: data remaining, used_timeout=   0.0                                                                                                                                    
2019-10-15 10:41:38 WARNING (SyncWorker_4) [root] GOT Response: {"accessories":[{"aid":1,"services":[{"characteristics":[{"format":"bool","iid":2,"perms":["pw"],"type":"14"},{"format":"string","iid":3,<snip>"}]}]}                                                                                                                                                                             
2019-10-15 10:41:38 WARNING (SyncWorker_2) [root] LIST_accessories_and_characteristics:  None                                                                                                                                            
2019-10-15 10:41:48 WARNING (SyncWorker_2) [root] STARTING NEW SESSION                                                                                                                                                                                                                                                                                      
2019-10-15 10:41:48 WARNING (SyncWorker_2) [root] handle request: b'GET /accessories HTTP/1.1\r\nHost: 192.168.0.53:7373\r\n\r\n'                                                                                                        
2019-10-15 10:41:48 WARNING (SyncWorker_2) [root] _handle_request len loop begin                                                                                                                                                         
2019-10-15 10:41:48 WARNING (SyncWorker_2) [root] unencrypted request data len: 54                                                                                                                                                       
2019-10-15 10:41:48 WARNING (SyncWorker_2) [root] encrypted request len: 72                                                                                                                                                              
2019-10-15 10:41:48 WARNING (SyncWorker_2) [root] encrypted request: b"6\x00\xd9\xf2\..."                                                                                                                                                                                                                                                                             
2019-10-15 10:41:48 WARNING (SyncWorker_2) [root] _read_response: loop begin...                                                                                                                                                          
2019-10-15 10:41:48 WARNING (SyncWorker_2) [root] _read_response: no data remaining, used_timeout=  10.0                                                                                                                                 
2019-10-15 10:41:48 WARNING (SyncWorker_2) [root] _read_response: loop begin...                                                                                                                                                          
2019-10-15 10:41:48 WARNING (SyncWorker_2) [root] _read_response: data remaining, used_timeout=   0.0                                                                                                                                    
2019-10-15 10:41:48 WARNING (SyncWorker_2) [root] GOT Response: {"accessories":[{"aid":1,"services":[{"characteristics":[{"format":"bool","iid":2,"perms":["pw"],"type":"14"},{"format":"string","iid": <snip> ,"type":"A2"}]}]}  

Then once a minute, the following logging statements repeat (with different encrypted request data). The request lengths are all the same (91/109).

2019-10-15 10:42:49 WARNING (SyncWorker_16) [root] handle request: b'GET /characteristics?id=1.101,1.102,1.103,1.104,1.107 HTTP/1.1\r\nHost: 192.168.0.53:7373\r\n\r\n'                                                                  
2019-10-15 10:42:49 WARNING (SyncWorker_16) [root] _handle_request len loop begin                                                                                                                                                        
2019-10-15 10:42:49 WARNING (SyncWorker_16) [root] unencrypted request data len: 91                                                                                                                                                      
2019-10-15 10:42:49 WARNING (SyncWorker_16) [root] encrypted request len: 109                                                                                                                                                            
2019-10-15 10:42:49 WARNING (SyncWorker_16) [root] encrypted request: b'[\x00"\x9b\xe8\x92\<snip>'                                                                                                                                                                                     
2019-10-15 10:42:49 WARNING (SyncWorker_16) [root] _read_response: loop begin...                                                                                                                                                         
2019-10-15 10:42:49 WARNING (SyncWorker_16) [root] _read_response: no data remaining, used_timeout=  10.0
2019-10-15 10:42:49 WARNING (SyncWorker_16) [root] _read_response: loop begin...
2019-10-15 10:42:49 WARNING (SyncWorker_16) [root] _read_response: data remaining, used_timeout=   0.0

I think there is a good chance that this Lennox Homekit firmware update may have also fixed the handshaking issues and that my successful pairing wasn't a random good luck thing. It sure seems to have fixed the bonjour broadcast/WAC/iOS requirement pairing issues.

garyak commented 4 years ago

Both my Smart Hubs were updated on 9/27 with 3.55.0219. No indications of pending firmware updates.

GaryOkie commented 4 years ago

double-checked... My Lennox SmartHub (model 105081-03) firmware is 3.55.0626, which matches the Homekit version reported via zeroconf to HA.

It shouldn't matter, but for reference, my S30 Thermostat (model 104553-01) firmware is 3.50.0148.

In the general/about menu, you can select automatic updates and check for software updates. My Tstat and Hub are both set for automatic updates, but this morning I saw a little red message notification icon on the main screen, which said a Homekit firmware update was available to install.

Jc2k commented 4 years ago

@GaryOkie i am reluctant to suggest it now that you have it working, but are you able to revert all changes and then try pairing again with a vanilla install?. If it works are you able to try again a couple of times? (You'll need to remove in from integrations menu in home assistant, restart HA and then use the S30 option to clear previous Homekit pairings.

garyak commented 4 years ago

Just for full disclosure, we have the same thermostats (104553-01), but different Smart Hubs. Mine are listed as 104329-01.

GaryOkie commented 4 years ago

Ok @jc2k - will do. As I had mentioned, the only code changes other than the logging was I kept the import time; time.sleep(10) delay in ip_implementation. I had already removed all the logging but I will remove the sleep as well and reset/repeat the pairing.

back soon with an update...

GaryOkie commented 4 years ago

Good news @jc2k - using production code, I was able to repeat the pairing process 3 times with no problems at all!

Jc2k commented 4 years ago

That is wonderful news indeed :-)

jeromelaban commented 4 years ago

It is working for me as well, paired on the first try! Thank you very much :)

GaryOkie commented 4 years ago

Would someone with an iComfort S30/M30/E30 please temporarily enable the "wider temperature setpoints" (under settings/heat&cool)? This will change the allowed range from 60-90 to 40-99 (F).

What is being reported to HA via the Homekit integration is an incorrect min_temp=60, instead of 40. The max_temp is correct at 99. I'm unable to set the temp lower than 60 via HA, but it works fine via the thermostat touchpad or iComfort app.

Can this min_temp reporting problem be duplicated by anyone?

Thanks!