Adjust locking to avoid data errors #97

bdraco · 2022-07-14T23:35:35Z

No description provided.

codecov · 2022-07-14T23:39:24Z

Codecov Report

Merging #97 (5ca6f2e) into main (d0cd358) will decrease coverage by 1.55%.
The diff coverage is 3.38%.

@@            Coverage Diff             @@
##             main      #97      +/-   ##
==========================================
- Coverage   68.18%   66.62%   -1.56%     
==========================================
  Files          67       67              
  Lines        5264     5397     +133     
==========================================
+ Hits         3589     3596       +7     
- Misses       1675     1801     +126

Flag	Coverage Δ
unittests	`66.62% <3.38%> (-1.56%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiohomekit/controller/ble/client.py	`0.00% <0.00%> (ø)`
aiohomekit/controller/ble/const.py	`0.00% <0.00%> (ø)`
aiohomekit/controller/ble/discovery.py	`0.00% <0.00%> (ø)`
aiohomekit/controller/ble/pairing.py	`0.00% <0.00%> (ø)`
aiohomekit/model/__init__.py	`96.15% <100.00%> (+0.10%)`	⬆️
aiohomekit/protocol/tlv.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0cd358...5ca6f2e. Read the comment docs.

bdraco · 2022-07-15T00:18:46Z

Getting the MTU doesn't seem to work

https://github.com/hbldh/bleak/blob/master/examples/mtu_size.py

2022-07-14 19:15:59.961 ERROR (MainThread) [homeassistant.config_entries] Error setting up entry  for homekit_controller                                                                       
Traceback (most recent call last):                                                                                                                                                           
  File "/usr/local/lib/python3.10/site-packages/bleak/backends/bluezdbus/client.py", line 533, in _acquire_mtu                                                                                               
    char = next(                                                                                                                                                                                           
StopIteration                                                                                                                                                                                      
                                                                                                                                                                                         
During handling of the above exception, another exception occurred:                                                                                                                            
                                                                                                                                                                                                                                                   
Traceback (most recent call last):                                                                                                                                                           
  File "/usr/local/lib/python3.10/site-packages/bleak/backends/bluezdbus/client.py", line 540, in _acquire_mtu                                                                               
    char = next(                                                                                                                                                                                                                                   
StopIteration                                                                                                                                                                                                                               
                                                                                                                                                                                                                   
The above exception was the direct cause of the following exception:                                                                                                                           
                                                                                                                                                                                                               
Traceback (most recent call last):                                                                                                                                               
  File "/usr/src/homeassistant/homeassistant/config_entries.py", line 356, in async_setup                                                                                                                      
    result = await component.async_setup_entry(hass, self)                                                                                                                       
  File "/usr/src/homeassistant/homeassistant/components/homekit_controller/__init__.py", line 228, in async_setup_entry                                                                              
    if not await conn.async_setup():                                                                                                                                            
  File "/usr/src/homeassistant/homeassistant/components/homekit_controller/connection.py", line 207, in async_setup                                                                                                
    if not await self.async_ensure_available():                                                                                                                            
  File "/usr/src/homeassistant/homeassistant/components/homekit_controller/connection.py", line 189, in async_ensure_available                                                             
    await self.pairing.get_characteristics([(aid, iid)])                                                                                                                               
  File "/usr/local/lib/python3.10/site-packages/aiohomekit/controller/ble/pairing.py", line 355, in get_characteristics                                                          
    await self._ensure_connected()                                                                                                                                                                         
  File "/usr/local/lib/python3.10/site-packages/aiohomekit/controller/ble/pairing.py", line 173, in _ensure_connected                                                                                  
    await self.client._acquire_mtu()                                                                                                                                                           
RuntimeError: coroutine raised StopIteration

bdraco · 2022-07-15T07:47:11Z

Seems to be mostly working now

the config num needs to be figured out since when you pair it has to sync 3 times now because we don't remember that

bdraco · 2022-07-15T07:53:14Z

aiohomekit/controller/ble/discovery.py

+            while not self.client.is_connected:
+                logger.debug("Connecting to %s", self.device)
+                try:
+                    await self.client.connect()
+                    break
+                except BleakError as e:
+                    logger.debug(
+                        "Failed to connect to %s: %s", self.client.address, str(e)
+                    )
+
+                if self.description.address != self.client.address:
+                    self.client = BleakClient(self.description.address)
+
+                await asyncio.sleep(5)


This probably shouldn't try forever

Yeah it blocks shutdown

2022-07-15 02:54:59.770 ERROR (MainThread) [homeassistant.components.hassio.handler] Timeout on /homeassistant/restart request
2022-07-15 02:55:06.038 DEBUG (MainThread) [aiohomekit.controller.ble.pairing] Failed to connect to FB:EC:42:A9:79:F9: Device with address FB:EC:42:A9:79:F9 was not found.
2022-07-15 02:55:11.040 DEBUG (MainThread) [aiohomekit.controller.ble.pairing] FB:EC:42:A9:79:F9: Connecting
2022-07-15 02:55:21.137 DEBUG (MainThread) [aiohomekit.controller.ble.pairing] Failed to connect to FB:EC:42:A9:79:F9: Device with address FB:EC:42:A9:79:F9 was not found.

Opened an issue so we don't forget #98

bdraco · 2022-07-15T07:53:39Z

aiohomekit/controller/ble/pairing.py

+        )
+
+    async def _establish_connection(self):
+        address = self.address
        while not self.client or not self.client.is_connected:


This probably shouldn't try forever

Opened an issue so we don't forget #98

bdraco · 2022-07-15T08:01:16Z

We also need to avoid this line for BLE devices and use the local name for the name so we don't read all the chars 2x
https://github.com/home-assistant/core/blob/b4003713b67d3e83397382f4af5d10e8d9a94660/homeassistant/components/homekit_controller/config_flow.py#L484

Jc2k · 2022-07-15T08:17:40Z

We also need to avoid this line for BLE devices and use the local name for the name so we don't read all the chars 2x

https://github.com/home-assistant/core/blob/b4003713b67d3e83397382f4af5d10e8d9a94660/homeassistant/components/homekit_controller/config_flow.py#L484

I've seen local name be too vague quite often. Eg for my own Eve devices their local name seems to be "Eve" (most of the time).

Think we should just make sure to only read the gatt database once and then retrieve the name from our cached copy so we don't even do a char read.

bdraco · 2022-07-15T15:05:28Z

We also need to avoid this line for BLE devices and use the local name for the name so we don't read all the chars 2x
home-assistant/core@b400371/homeassistant/components/homekit_controller/config_flow.py#L484

I've seen local name be too vague quite often. Eg for my own Eve devices their local name seems to be "Eve" (most of the time).

Think we should just make sure to only read the gatt database once and then retrieve the name from our cached copy so we don't even do a char read.

👍 I'll make getting the cache working a priority in the next PR

Jc2k · 2022-07-15T15:50:36Z

aiohomekit/controller/ble/pairing.py

+                CharacteristicsTypes.PAIR_VERIFY,
+                get_session_keys(self.pairing_data, self._session_id, self._derive),
+            )
+        # FIXME: this should not be a broad except handler


I fixed the obvious bug in resume_m3, so I don't think we need this try/except block any more.

The resume handshake is supposed to be graceful in that if a device doesn't support resume or resume fails, the device will act as though we are already doing a full pair verify. So we should just be able to code defensively and fall through into a normal pair resume. There is logic to do that, but i wasn't handling the case where the method field wasn't present at all, which I do now.

Will adjust in the next run

Adjust locking to avoid data errors

1d07cec

Jc2k approved these changes Jul 14, 2022

View reviewed changes

bdraco added 3 commits July 14, 2022 18:51

more fixes

4348e75

hold the lock for closing as well

de42ba6

fix mtu

55fb424

bdraco added 23 commits July 14, 2022 19:42

fixes

f546534

fixes

06ab219

fixes

3693de8

fixes

e285b8c

fixes

06db423

fixes

0478dc5

fixes

daea226

fixes

8baa7ff

fixes

df3d372

fixes

a34d1d1

fixes

cc32fcf

fixes

bae3ab6

fixes

f19c0d6

fixes

05d2bd8

fixes

2cf61d7

fixes

413c89f

fixes

a66bb7d

fixes

48343f1

fixes

8f80ab7

fixes

01dab9c

fixes

0472019

fixes

deaa49f

handle agressive disconnects

4cf175b

bdraco added 4 commits July 15, 2022 02:09

more fixes

92fd443

more fixes

e26e451

tweak

b6c433a

more fixes

8008258

tweak

c28f7aa

bdraco commented Jul 15, 2022

View reviewed changes

bdraco added 2 commits July 15, 2022 03:03

do not read pairing

e43812d

do not read pairing

9cd7ee8

bdraco added 4 commits July 15, 2022 03:19

do not read pairing

b5074c1

track the config and state together

7b9b98f

track the config and state together

a611357

Merge branch 'main' into locking

623efbe

bdraco added 3 commits July 15, 2022 10:05

do not read pairing

cefe366

do not read pairing

a61dc6d

add missing

06b6f8c

bdraco mentioned this pull request Jul 15, 2022

BLE connections try forever and block HASS shutdown #98

Closed

bdraco added 4 commits July 15, 2022 10:12

preen

63f3089

preen

031ec23

preen

1322876

track the config and state together

5ca6f2e

bdraco marked this pull request as ready for review July 15, 2022 15:23

Jc2k reviewed Jul 15, 2022

View reviewed changes

bdraco mentioned this pull request Jul 15, 2022

Test if we can remove the resume pairing fallback #99

Closed

bdraco merged commit f817cef into main Jul 15, 2022

bdraco deleted the locking branch July 15, 2022 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust locking to avoid data errors #97

Adjust locking to avoid data errors #97

bdraco commented Jul 14, 2022

codecov bot commented Jul 14, 2022 •

edited

bdraco commented Jul 15, 2022

bdraco commented Jul 15, 2022

bdraco Jul 15, 2022

bdraco Jul 15, 2022

bdraco Jul 15, 2022

bdraco Jul 15, 2022

bdraco Jul 15, 2022

bdraco commented Jul 15, 2022

Jc2k commented Jul 15, 2022

bdraco commented Jul 15, 2022

Jc2k Jul 15, 2022 •

edited

bdraco Jul 15, 2022

bdraco Jul 15, 2022

Adjust locking to avoid data errors #97

Adjust locking to avoid data errors #97

Conversation

bdraco commented Jul 14, 2022

codecov bot commented Jul 14, 2022 • edited

Codecov Report

bdraco commented Jul 15, 2022

bdraco commented Jul 15, 2022

bdraco Jul 15, 2022

Choose a reason for hiding this comment

bdraco Jul 15, 2022

Choose a reason for hiding this comment

bdraco Jul 15, 2022

Choose a reason for hiding this comment

bdraco Jul 15, 2022

Choose a reason for hiding this comment

bdraco Jul 15, 2022

Choose a reason for hiding this comment

bdraco commented Jul 15, 2022

Jc2k commented Jul 15, 2022

bdraco commented Jul 15, 2022

Jc2k Jul 15, 2022 • edited

Choose a reason for hiding this comment

bdraco Jul 15, 2022

Choose a reason for hiding this comment

bdraco Jul 15, 2022

Choose a reason for hiding this comment

codecov bot commented Jul 14, 2022 •

edited

Jc2k Jul 15, 2022 •

edited