-
-
Notifications
You must be signed in to change notification settings - Fork 190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for alternative discovery protocol (20002/udp) #488
Add support for alternative discovery protocol (20002/udp) #488
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #488 +/- ##
==========================================
+ Coverage 79.13% 79.42% +0.28%
==========================================
Files 26 26
Lines 1989 2041 +52
Branches 613 628 +15
==========================================
+ Hits 1574 1621 +47
Misses 373 373
- Partials 42 47 +5
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @sdb9696 and sorry for the delay, I'm on holidays but managed to give this a quick test against a tapo plug and it gets detected fine:
Found unsupported device (tapo or device using newer encryption protocol): {'result': {'device_id': 'xx', 'owner': 'xx', 'device_type':
'SMART.TAPOPLUG', 'device_model': 'P110(EU)', 'ip': '192.168.1.208', 'mac': '48-22xxx', 'is_support_iot_cloud': True, 'obd_src': 'tplink', 'factory_default': False, 'mgt_encrypt_schm':
{'is_support_https': False, 'encrypt_type': 'AES', 'http_port': 80, 'lv': 2}}, 'error_code': 0}
See my comments inline for some minor changes that I think should be done prior this gets merged.
893bc1c
to
4fb5c42
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I spend some time on experimenting with ideas from this PR and the one adding support for a new protocol, and created preliminary support for tapo devices (well, a plug that I have as a test device, with hardcoded creds..) by creating a TapoPlug(SmartPlug)
shim that uses https://github.com/petretiandrea/plugp100 (friendly ping @petretiandrea) for device communications. I'll create a draft PR for that soon-ish.
As this uses a different protocol altogether, it was easy to create a separate "shim" to add support for those devices. I was thinking if it would make sense to keep the klap protocol in a separate library (mainly to avoid new dependencies for this library), but I'm unsure about given the protocol is exactly the same but only differs on the transport layer... What do you think?
Anyway, the next steps no matter how we want to proceed would be:
- Implementing
discover_single
is needed to add support for homeassistant. I think this should be done as part of this PR to make it a complete in terms of discovery support. I'm wondering what's the best approach for handling passing the credentials. Perhapsdiscover_single()
should raise an exception to direct downstreams to pass the creds, if not given. This way the config flow can request the information only when it's necessary. - Make
SmartDevice
(and subclass) ctors to accept authentication credentials (this could be a single PR, this would unblock me to experiment further with tapo integration). The current implementation can just ignore this, i.e., just store it for later uses.
4fb5c42
to
8d3b0ae
Compare
I've made the simplification changes you suggested so hopefully all good now.
I think it works well to have both original and klap derive from a base class because little else needs to change. If you look at the changes I made to #477 following your review it's very much simplified and lower impact. Only
I have an implementation of discover single in #477. I'm not sure how it would work in this PR because you can't query sysinfo without the klap changes. I have a fork of the ha tp-link component working well with the klap PR but my config flow needs improving a bit. At the moment it just allows you to set the username/password if you want to. Before I removed the unauthenticated_device from the klap PR HA would display the device to indicate to the user that they should store some credentials.
I really think the protocol should be passed to the SmartDevice (and subclasses) ctors and the authentication belongs to the protocol. If you look at my discover_single implementation it's discovery which determines the correct protocol to initialise the device with (could be original, klap or tapo). The authentication credentials by themselves do not decide whether it's klap or tapo for example. Out of interest do you know whether the KP125M might be supporting TAPO type protocol with exactly the same queries as the other kasa devices? I'd considering buying one to test whether porting the TAPO protocol to work with the changes proposed in the klap PR would work. |
Looking great (aside the missing discover_single), thanks!
Agreed, I think that's a fair approach. I'm unsure if this necessitates introducing the
We could use the discovery information to trigger an exception, the tapo devices have a specific model among other things in the payload that could be used for that: #488 (review) . How does a discovery response look for your UK devices? I'm thinking about having a following logic:
To my understanding, we have enough information available in the discovery to pull this off, right?
So the thing with tapo plugs is that it isn't using the same set of commands, so the protocol approach does not work in that case. I created a PR for my PoC #499 to show how it works currently. What we could do wrt.
How does that sound to you?
Alas, I don't know. #450 is the issue for KP125M, but I recall seeing somewhere that it does not use the klap so it might very well be using the tapo one. edit: looks like #422 (comment) says it sports the tapo api, so you should be good to go to use that for testing, @sdb9696 :-) |
It sounds ok but I just want to double check something with you before I go ahead. Currently the discover_single does a
I have an example at the top of #477. V. similar to TAPO so should support your suggestions. |
Oh, I didn't realize it is not using the UDP for discovery, although maybe it should... The I would say we go ahead and use UDP for both types of discoveries, we can always revert it back for "legacy" discovery if it doesn't work out. But IIRC, we should have enough information in those responses to perform the model detection, so feel free to go ahead. |
Ok so PR now uses UDP for discover_single. Tested with physical kasa devices with the original and the klap protocol. Added some tests for this. I had to make some minor tweaks for the --port override feature as it was only implemented to work with discover_single and not discover. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep the unsupported device handling simple, other than that this is good to go.
I rebased the tapo draft on top of this for testing, see 1676b7a for how I modified the _get_device_class
, for the klap we
e860e4a
to
be2ed61
Compare
…d of a direct query
be2ed61
to
b17c645
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let's merge and refine where needed in the future PRs. Thanks again! 🥇
This PR will broadcast the new discovery message on the new port and log any responses received as unsupported devices.