Add klap protocol

[sdb9696]

This pull request is a new copy of PR #477 which was broken into multiple smaller PRs (#488 & #507) to prepare the way for supporting the kasa klap protocol and potentially other methods of authentication. PR 477 built on the work done by @SimonWilkinson #117.

Background

KASA introduced the new klap protocol in Nov 2020 #115 via an automatic firmware update that largely affected HS100 models in the UK (and perhaps Italy). Whilst tplink stopped the rollout due to complaints from home assistant users it should be noted that tplink recently rolled out an update to klap v2 to some of their TAPO devices which stopped them working in a similar fashion.

Notes

Tests will be added once the PR is broadly agreed

Discovery responses

kasa klap:

{'result': {'ip': '192.168.123.123', 'mac': '12-34-56-78-9A-BC', 'device_id': '123456789ABCDEF123456789ABCDEF12', 'owner': 'fedcba987654321fedcba987654321fe', 'device_type': 'IOT.SMARTPLUGSWITCH', 'device_model': 'HS100(UK)', 'hw_ver': '4.1', 'factory_default': True, 'mgt_encrypt_schm': {'is_support_https': False, 'encrypt_type': 'KLAP', 'http_port': 80}}, 'error_code': 0}

TAPO klap:

 {'result': {'device_id': '****', 'owner': '****', 'device_type': 'SMART.TAPOBULB', 'device_model': 'L530E(EU)', 'ip': '****', 'mac': '*****', 'is_support_iot_cloud': True, 'obd_src': 'tplink', 'factory_default': False, 'mgt_encrypt_schm': {'is_support_https': False, 'encrypt_type': 'KLAP', 'http_port': 80, 'lv': 2}}, 'error_code': 0}

N.B.the discovery response for a kasa klap device doesn't have 'lv: 2.

Fixes #115

[codecov]

Codecov Report

Attention: 45 lines in your changes are missing coverage. Please review.

Comparison is base (bde07d1) 80.46% compared to head (f5d8041) 81.63%.

Files	Patch %	Lines
kasa/klapprotocol.py	92.20%	5 Missing and 13 partials ⚠️
kasa/cli.py	13.33%	12 Missing and 1 partial ⚠️
kasa/discover.py	83.11%	8 Missing and 5 partials ⚠️
kasa/smartdevice.py	94.11%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #509      +/-   ##
==========================================
+ Coverage   80.46%   81.63%   +1.16%     
==========================================
  Files          27       28       +1     
  Lines        2104     2417     +313     
  Branches      639      683      +44     
==========================================
+ Hits         1693     1973     +280     
- Misses        366      383      +17     
- Partials       45       61      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rytilahti]

Great work on those other PRs, this PR is now so much easier to review! :-)

I added some comments, but could you please add some initial tests? Even initializing the klapprotocol class without any real tests would make it much easier to read as codecov would not warn on every code block.

[sdb9696]

Gentle nudge @rytilahti

[rytilahti]

Sorry for keep you waiting @sdb9696, I was traveling and too busy with other things but I finally sat down and took a look into the klapprotocol, and added plenty of comments on how to make the code cleaner and easier to understand (and to maintain, I hope :-)).

I'll do another review with fresh eyes later, but let's start getting it cleaned for the next release :-)

[sdb9696]

Sorry for keep you waiting @sdb9696, I was traveling and too busy with other things but I finally sat down and took a look into the klapprotocol, and added plenty of comments on how to make the code cleaner and easier to understand (and to maintain, I hope :-)).

I'll do another review with fresh eyes later, but let's start getting it cleaned for the next release :-)

Hi @rytilahti, apologies for the delay. I've integrated nearly all of your comments so far. There are a couple still open with replies.

[rytilahti]

Some more comments. I thought I wrote at some point about cleaning up the session handling to avoid constructing a session object for every _execute_query call and passing it around, but maybe that got lost during a rebase?

The code would become immediately much simpler, if the session would be initialized only when one does not exist (or errors) and stored in self.session to be used. This would follow the same, much more optimized approach like the regular protocol does, i.e., keep the connection to the device open to avoid the set-up overhead. Basically, using a pattern like done in the current protocol: have _connect that performs protocol initialization and implementing close() which resets the state when necessary (e.g., on errors) so that the connect will reinitialize it again.

Maybe that would also simplify the state handling, at the moment there are way, IMO, too many state instance variables which makes following the code quite hard and likely prone to errors. If you want, I could try to give that a go over the weekend and create a PR for you to test on a real device?

[sdb9696]

Some more comments. I thought I wrote at some point about cleaning up the session handling to avoid constructing a session object for every _execute_query call and passing it around, but maybe that got lost during a rebase?

The code would become immediately much simpler, if the session would be initialized only when one does not exist (or errors) and stored in self.session to be used. This would follow the same, much more optimized approach like the regular protocol does, i.e., keep the connection to the device open to avoid the set-up overhead. Basically, using a pattern like done in the current protocol: have _connect that performs protocol initialization and implementing close() which resets the state when necessary (e.g., on errors) so that the connect will reinitialize it again.

I have now implemented this so that the httpx.AsyncClient is a class instance variable and remains connected until protocol.close() or on error. (There is no need with the AsyncClient to explicitly call connect as it handles it internally.)

Also I realised I had missed a bunch of other comments from you on the klapprotocol class. I have now addressed all of these so I'd like to think we're nearly there :)

[sdb9696]

Hi @rytilahti I hope you are well. Do you think we could get this closed out soon? I agree with your comments on the HA PR and I’m ready to get that sorted once this is merged.

[rytilahti]

Hi @sdb9696 and sorry for the delay, I am currently terribly, terribly busy IRL (and will remain so for the time being) and I haven't had free cycles to think on this project for a while...

Anyway, I went through the current state briefly and I think the PR looks now so much better, thanks! So I feel that we should just merge this after the comments I added now are done, and fix whatever comes up in separate PRs.

[sdb9696]

Ok @rytilahti that's all fully done except for the one item above for the reason given.

[rytilahti]

LGTM, thanks a lot @sdb9696! 💯