Codecov Report

Patch coverage: 82.35% and project coverage change: +0.28% 🎉

Comparison is base (53021f0) 79.13% compared to head (b17c645) 79.42%.

@@            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.
📢 Have feedback on the report? Share it here.

I've made the simplification changes you suggested so hopefully all good now.

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?

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 query and close are in the abstract base protocol class.

• 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. Perhaps discover_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.

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.

• 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.

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.

I've made the simplification changes you suggested so hopefully all good now.

Looking great (aside the missing discover_single), thanks!

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 query and close are in the abstract base protocol class.

Agreed, I think that's a fair approach. I'm unsure if this necessitates introducing the protocol to the ctors, or if we could just inject it in discover_single into its place? That would furthermore simplify it. See my comment wrt. discover_single below on a potential approach.

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.

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:

1. Homeassistant calls discover_single like it does now
2. We check the discovery payloads to see if credentials are required. If not, we construct and return the device instance.
3. If required and no credentials parameter is given, raise an exception which triggers the request to input creds (if not stored already by homeassistant, no idea what's the best approach to store and share this among devices yet, though..). I think there is a separate way to signal homeassistant for "repairs" to do just that.
4. Homeassistant retries discover_single with the given creds, raising exception if they are not valid (i.e., we cannot construct and query the device)
5. If creds work and we get an update, we return the device instance as currently.

To my understanding, we have enough information available in the discovery to pull this off, right?

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.

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. discover_single is to use the discovery responses in the following order:

1. If we get the kasa-response, we are good to go
2. If we receive new discovery response, check if the device_type contains TAPO. If creds are given, we can try to initialize the tapo implementation
3. Lastly, we are encountering these UK devices. Here, we will construct a smartdevice, set its protocol to the newly created KLAP one and try to fetch its info

How does that sound to you?

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.

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 :-)

What we could do wrt. discover_single is to use the discovery responses in the following order:

1. If we get the kasa-response, we are good to go
2. If we receive new discovery response, check if the device_type contains TAPO. If creds are given, we can try to initialize the tapo implementation
3. Lastly, we are encountering these UK devices. Here, we will construct a smartdevice, set its protocol to the newly created KLAP one and try to fetch its info
   How does that sound to you?

It sounds ok but I just want to double check something with you before I go ahead. Currently the discover_single does a {"system": {"get_sysinfo": None}} query to the device to get the basic discovery info then a further dev.update() to get the extra info. AFAIK the only way to get the initial tapo/klap discovery response is to UDP broadcast the magic number and receive the datagram back. To implement your suggestion above I think I would have to modify the initial step to do a UDP broadcast on 9999/20002 instead of a direct query to 9999 for the get sysinfo. That way the logic can flow as per your suggestion for 1, 2, 3. Does that make sense and are you ok with that change?

How does a discovery response look for your UK devices?

I have an example at the top of #477. V. similar to TAPO so should support your suggestions.

It sounds ok but I just want to double check something with you before I go ahead. Currently the discover_single does a {"system": {"get_sysinfo": None}} query to the device to get the basic discovery info then a further dev.update() to get the extra info. AFAIK the only way to get the initial tapo/klap discovery response is to UDP broadcast the magic number and receive the datagram back. To implement your suggestion above I think I would have to modify the initial step to do a UDP broadcast on 9999/20002 instead of a direct query to 9999 for the get sysinfo. That way the logic can flow as per your suggestion for 1, 2, 3. Does that make sense and are you ok with that change?

Oh, I didn't realize it is not using the UDP for discovery, although maybe it should... The datagram_received uses the discovery information to construct device instances (since #132), but I don't have currently access to any of kasa-devices to check out what exactly is being returned for the UDP query.

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.

Oh, I didn't realize it is not using the UDP for discovery, although maybe it should... The datagram_received uses the discovery information to construct device instances (since #132), but I don't have currently access to any of kasa-devices to check out what exactly is being returned for the UDP query.

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.