Codecov Report

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

Comparison is base (ec3ea39) 84.83% compared to head (9325ba5) 85.01%.

@@            Coverage Diff             @@
##           master     #569      +/-   ##
==========================================
+ Coverage   84.83%   85.01%   +0.17%     
==========================================
  Files          37       38       +1     
  Lines        3231     3389     +158     
  Branches      810      851      +41     
==========================================
+ Hits         2741     2881     +140     
- Misses        413      422       +9     
- Partials       77       86       +9     

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

N.B. internal DeviceType is not included in ConnectionParameters as it made it complicated to set them on a device from discovery (Because the device has it's own _deviceType variable)

I think this is fine, we can use the internal one to match the interfaces in the future.

I have a homeassistant branch locally that incorporates @bdraco's home-assistant/core#104213 along with the connection_params and authorisation data flows. I need to finalise it tomorrow morning and then I'll create a PR and link it here.

W.r.t cli.py what are your thoughts on whether the cli takes the underlying connection arguments (i.e. --encryption-type etc) as separate parameters or a single --connection-params with the json string? I prefer separate underlying arguments myself for useability.

Great. Can you send a PR to my HA core branch? I'll merge it in and test

Great. Can you send a PR to my HA core branch? I'll merge it in and test

To the dev branch here?

https://github.com/home-assistant/core/tree/tplink_save_device_class_connect

W.r.t cli.py what are your thoughts on whether the cli takes the underlying connection arguments (i.e. --encryption-type etc) as separate parameters or a single --connection-params with the json string? I prefer separate underlying arguments myself for useability.

It makes sense to have them as separate options to avoid manually building json objects which can be a hassle. I hope having those parametrized will make it easier for someone to start looking into #422 and #467 using the raw-command for queries.

HA PRs are ready. This is still work in progress from a testing point of view but feedback welcome. @bdraco it would be good if you can put any comments on home-assistant/core#105143 so they're all in the same place.

home-assistant/core#105144 for @bdraco

home-assistant/core#105143 for @rytilahti and @ezekieldas

@ezekieldas the way I usually test HA core changes is:

1. Follow the setup local repository instructions
2. Change the http port in config/configuration.yaml so I can run it alongside my production hass:

http:
  server_port: 8125

3. Run hass once with hass -c config so it installs any dependencies it needs.
4. Stop hass.
5. Checkout the python-kasa and the homeassistant/core PRs
6. Uninstall python-kasa pip uninstall python-kasa
7. Install the local code pip install -e ../path_to_where_you_have_checked_out_python_kasa_pr
8. Run hass again with hass -c config --skip-pip-packages python-kasa so it doesn't try to re-install the version that was uninstalled.

To checkout PRs (origin if you cloned, upstream if you forked):

cd DIRECTORY_WHERE_YOU_CLONED_OR_FORKED
git fetch [origin|upstream] pull/PRNUM/head:pullPRNUM
git checkout pullPRNUM

EDIT @ezekieldas I've updated step 7 (now 8) to only skip installing the python-kasa package. This will be needed for when I rebase to dev branch and hass needs to update other packages. Also step 1 & 3 could run for a while so you might want to leave it installing and come back to it. Also put a reminder to checkout the PR.

FYI I have added optional functionality for device_factory.connect() to try discovery if the connection fails. This is because otherwise if the connection parameters are persisted and the device upgrades it's firmware to Klap or something else there needs to be a way to update the config_entry ConnectionParameters so the device will work again without having to delete and recreate.

In order to be able to do this without circular references the three get_device_class_protocol_blah methods needed to be moved into a different module.

So I skimmed over some of this PR (it's quite a lot of changes already.. :-D) and wanted to give some early feedback and potential ideas for improvements, much of this is rather abstract but I think we are in a position where we need to discuss about the mechanics to avoid issues down the path when it's more complicated to avoid breaking things.

Apologies things grew a bit to get it working properly with HA. With regard to breaking things I think in the short term we want to be able to change the kasa.tapo package without worrying about who consumes it (for example we may want to move or rename it or change the public methods). I suggest we add a note to the README stating that the support is experimental and consumers should not expect the api to be stable for a while (or even _underscore the modules).

So far HA is not really aware of the kasa.tapo package (especially when we remove the _device_type as above) so I propose we minimise the scope of this PR and 105143 to:

1. Use ConnectionParameters and/or device_type to reduce the number of update() calls HA needs to make when setting up config entries.
2. Enable authentication and communication with IOT.KLAP, SMART.AES and SMART.KLAP devices.
3. Provide basic support for SMART.KASAPLUG, SMART.TAPOPLUG & SMART.TAPOBULB. We could update the HA docs to explain this is experimental and even mention it in the config flow.

Basically, what I have in my mind is that we need to thinkg about defining some "basic device types" (plugs, bulbs, dimmers, powerstrisp) which implement their corresponding interfaces. Any additional information and features would be implemented in an abstracted way by adding some metadata to implementations that lets us to find out and access features without hardcoding them.

One of the things with the SMART protocol is there is minimal information in the discovery response to determine the "basic device types", i.e. we only have the tplink device type of "SMART.BLAH" which may end up only having the same 2 types as IOT, i.e. bulb or plugswitch. We might want to keep the number of basic types in line with tplink and add children capability to TapoDevice and treat the dimmer as a TapoPlug with a DimmerSwitch feature. What do you think?

I had some trouble with this but eventually got everything sorted out (see further below). Most important to note, and was the case with previous tests, is I had to set env http_proxy='' prior to running hass -c config --skip-pip. I won't elaborate here but I've had trouble with HA and having forward proxies enforced in my environment(s).

Anyhow, this is the end result and it's fantastic!

Issues:

• I overlooked the necessary checkout on core after Setup Local Repository
  • do gh pr checkout 105143 after git clone
• I had the same trouble as Bogdan Sass as noted here:
  Setting up local dev env - “no such option –config-settings”
  • I ultimately found it best to use python3.11 exactly

Great to hear it's working for you @ezekieldas. I have updated both PRs to fix some useability and architectural issues. If you do check them both out again you should make sure to see my update to the instructions above to use --skip-pip-packages python-kasa to keep things in sync with the rest of HA.

Thanks for taking the time!

FYI I have rebased this PR on top of a new PR #578 to use the tapo error_codes to determine whether to retry or fail etc. @rytilahti it would probably make sense to look at #578 first as it's fairly simple and will probably help you with your own testing.

So far HA is not really aware of the kasa.tapo package (especially when we remove the _device_type as above) so I propose we minimise the scope of this PR and 105143 to:

HA nor any other downstream should never be aware of our internals :-)

1. Use ConnectionParameters and/or device_type to reduce the number of update() calls HA needs to make when setting up config entries.

I'll try to go through this PR directly after the error code PR is merged and this is rebased on top of the master branch. But yeah, the only interface HA should be aware of (in my opinion) is that there's a way to get "connection parameters" out from an existing instance and constructing a device instance again by simply using that data.

2. Enable authentication and communication with IOT.KLAP, SMART.AES and SMART.KLAP devices.

Yup, and this is looking really good based on my testing with this PR and the one for HA already :-)

3. Provide basic support for SMART.KASAPLUG, SMART.TAPOPLUG & SMART.TAPOBULB. We could update the HA docs to explain this is experimental and even mention it in the config flow.

We don't want any mentions about such low-level things, this is all purely something that just magically happens in the background and should not matter to users: the integration will just suddenly start providing support for more devices (in exchange for creds) and that's about it.

One of the things with the SMART protocol is there is minimal information in the discovery response to determine the "basic device types", i.e. we only have the tplink device type of "SMART.BLAH" which may end up only having the same 2 types as IOT, i.e. bulb or plugswitch. We might want to keep the number of basic types in line with tplink and add children capability to TapoDevice and treat the dimmer as a TapoPlug with a DimmerSwitch feature. What do you think?

The children is already part of the API, and will remain also if we introduce new base classes for those basic device types. I don't think we should worry too much about the devices that we do not possess (e.g., those dimmers which may function differently to kasa ones), as when we get a new release out that outputs useful "not supported" information there will be issue reports (or even PRs) coming in about not supported devices.

#578 (comment) - Do you see this as a new optional parameter on SmartDeviceException or a new SmartProtocolException type?

I'll try to go through this PR directly after the error code PR is merged and this is rebased on top of the master branch. But yeah, the only interface HA should be aware of (in my opinion) is that there's a way to get "connection parameters" out from an existing instance and constructing a device instance again by simply using that data.

error_core PR now rebased into this PR. I'll look at removing device_type parameter and updating cli discover tomorrow but if you get a chance today to add any other review comments to this PR that'd be great as then I can do it all in one batch.

The multi-request PR is nearly ready to submit once we're done with this one (I think you'll like it as it includes the query_helper type behaviour you requested). After that we do the feature interface PR and I reckon we're nearly there for the next release version and the HA PR :-O

Now rebased on top of #584 so in theory ready to go :D

Hi @rytilahti, I've held off on submitting any more PRs so we can hopefully focus on finishing this one off before the holidays.

In the meantime I've managed to get hold of a L510 and prevent it updating so I have an AES device again. Also I've managed to get this dynamic_light_effect update working locally so just in case you were going to spend time on it I can share the solution.

Hi @rytilahti, I've held off on submitting any more PRs so we can hopefully focus on finishing this one off before the holidays.

Hi @sdb9696 and happy holidays, feel free to create new PRs as you see fit. I've already traveled for the holidays and I may or may not have some free cycles in between to do some brief reviews :-)

In the meantime I've managed to get hold of a L510 and prevent it updating so I have an AES device again. Also I've managed to get this dynamic_light_effect update working locally so just in case you were going to spend time on it I can share the solution.

Great, thanks for the notice 👍 feel free to create a PR for that, I'm rather unlikely to have spare cycles to do any coding at the moment but I can sneak in a review or two now and then.

if there are ctors taking in port as an argument, we should remove those in favour of the config.

That's done now for all the SmartDevice classes. N.B. host is still positional but we may want to make it keyword to be consistent with the SmartDevice.Connect() method. Also Discovery still takes ports and credentials as parameters as per your previous direction so I'm assuming you still want it that way.

Most of the suggestions now implemented. A couple of open questions with replies which we can hopefully discuss post-merge. One below I couldn't reply to directly:

I think connect() on smartdevice class should simply pass the incoming parameters to the factory, which should avoid this exact problem, no?

Yes that's now done

LGTM, this is a great cleanup, thanks! 💯

I'm wondering if we should keep some of the existing parameters in the constructors but rather deprecate them for a release or two, but that's something which can be done in a separate PR if we feel that's worth having..

So, juts a couple of things to do and then this is ready to go:

1. Please update the PR description with a "Breaking change" block to help writing the release notes & help someone coming in to look why their code broke and how to fix it.

Done.

2. Update the documentation on initialization https://python-kasa.readthedocs.io/en/latest/design.html#initialization (and potentially, add a new section about describing the DeviceConfig).

Done. I'd like to spend a bit more time on the docs after this is merged but I think we have enough for now.