Open questions about TAPO/SMART implementation

[sdb9696]

I'm not sure if this is the right place for it but as we have been implementing the new TAPO type protocol for TAPO and some KASA devices there have been some open questions about future changes. I'm starting this discussion so we can capture and not lose sight of them.

Some communications feel rather sluggish, due to multiple queries (device state, then the emeter & the usage). Maybe these could be condensed into a single request? Just a thought.

EDIT: This is addressed with multipleRequest

The bulb seems to support these commands, too, but I'm not sure if we can detect the availability using component_nego or some other way.

EDIT: Now using component nego

Just a note for future, so no need for changes now, but we may want to move the mapping about supported device types inside the implementation classes

Captured discussion about interfaces

[rytilahti]

Good idea! I don't think it matters much whether we use this or an issue for collecting useful information, here's some more:

• The L530 becomes unresponsive quite often after testing commands, requiring a hard reset.
• We should not retry on known, device-reported errors. For example, retrying to login with same creds on -1501 (invalid creds error) doesn't help. Same applies to other errors like for invalid/unknown commands (-1002), -1601 (trying to call get_latest_fw too early, maybe the cloud connection is still initialization) and probably others, too.
• Create an enum/mapping for known error codes, the libraries linked below have figured out at least some of them.
• raw-command (and _query_helper) needs to be modified to allow passing raw strings.
• We probably want to create device-type specific interfaces to keep the implementations in sync.
• How to provision these without the app?

OUIs for homeassistant dhcp discovery

• L530: 5c:e9:31
• P110: 48:22:54

Some commands & responses that are working

There are more, but here's some that worked on L530:

• device_reboot: null
• get_connect_cloud_state: {"status": 1} when disconnected, 0 when connected.
• get_wireless_scan_info: {"ap_list": [], "wep_supported": False}
• get_fw_download_state: {"status": 0, "download_progress": 0, "reboot_time": 5, "upgrade_time": 5, "auto_upgrade": false}
• get_latest_fw: {"type": 2, "fw_ver": "1.1.0 Build 230823 Rel.163903", "release_date": "2023-09-01", "release_note": "Modifications and Bug Fixes:\n1. Enhanced stability and performance.\n2. Enhanced the local communication security.\n3. Fixed some minor bugs.", "fw_size": 786432, "hw_id": "xx", "oem_id": "xx", "need_to_upgrade": true}
• get_ffs_info: {"pubkey": "", "prikey": "", "pin": "ETosFj1Ju"}

Related projects

• Tapo (rust): https://github.com/mihai-dinculescu/tapo
• Tapo plug rest api: https://gitlab.com/0xSamy/TapoPlug-Rest-API
• plugp100: https://github.com/petretiandrea/plugp100

[sdb9696]

The L530 becomes unresponsive quite often after testing commands, requiring a hard reset.

I believe this is now addressed

We should not retry on known, device-reported errors. For example, retrying to login with same creds on -1501 (invalid creds error) doesn't help. Same applies to other errors like for invalid/unknown commands (-1002), -1601 (trying to call get_latest_fw too early, maybe the cloud connection is still initialization) and probably others, too.

This is complete

Create an enum/mapping for known error codes, the libraries linked below have figured out at least some of them.

This is complete

raw-command (and _query_helper) needs to be modified to allow passing raw strings.

This is complete

We probably want to create device-type specific interfaces to keep the implementations in sync.

Superseded with the new interface work

How to provision these without the app?

This is complete

[sdb9696]

From @rytilahti #551 (comment)

This looks good to me and worth merging.

My only worry is that adding such "low-level" things (like importing custom protocols / initializing them) may not be acceptable for homeassistant..

So I was thinking, maybe we could create a very simple container for all necessary connection parameters (ConnectionParameters or something like that?), and make it possible to pass only that for constructing instances. W/o pydantic if that's a deal breaker, although I doubt it should be a problem in a non-nested model).

As long as we keep this backwards compatible, we can easily extend it without needing to touch homeassistant code. Basically, SmartDevice should be able to output this json serializable object, like @bdraco mentioned, as well as accept it to re-create the instance.

What we basically need is the following information to construct a device from zero, right?

• Host
• Port
• Device type
• Protocol (legacy, klap, aes)?
• Username (if applicable)
• Password (if applicable)
• Timeout

What do you think?

So for HA I think username, password and timeout would be integration level settings rather than per config entry / device. I will very soon be updating my HA KLAP PR to handle TAPO store the username/password centrally in the Store for the integration and rely on re-auth flow to update (as per your suggestion here.

Regarding persisting connection parameters it might be simpler just to store them as [IOT|SMART].ENCRYPT_TYPE strings along the lines of the logic here. It could be as simple as that forever and if it ever gets more complicated we could add items to the end of the string (like if they enable https for example). However I'm just as open to a simple ConnectionParameters container that has PROTOCOL_NAME, ENCRYPT_TYPE etc. @bdraco do you have a view? It would be good to nail this one down so we can start bringing the unsupported KASA devices back into HA and also get TAPO ones underway.

N.B. does anyone have thoughts on passing the device_type string as returned from the device directly to the device_factory instead of the libraries own enum?

[rytilahti]

So for HA I think username, password and timeout would be integration level settings rather than per config entry / device.

Agreed, that sounds like a good idea at least to begin with. It may still be necessary to store this information to the config entries? But let's start simple and iterate if needed.

Regarding persisting connection parameters it might be simpler just to store them as [IOT|SMART].ENCRYPT_TYPE strings along the lines of the logic here.
I don't really like the idea of encoding structured information into a string to be parsed later on. It feels just error-prone and not as future-proof as using a dict to store the parameters and let the library decide on it.

N.B. does anyone have thoughts on passing the device_type string as returned from the device directly to the device_factory instead of the libraries own enum?

On device_type, I think this should be part of the connection params. For legacy this seems to be always IOT.SMARTPLUGSWITCH or IOT.SMARTBULB, even for led strips, power strips, and dimmers, so we may still need to pass extra type information somehow :-(

[sdb9696]

On device_type, I think this should be part of the connection params. For legacy this seems to be always IOT.SMARTPLUGSWITCH or IOT.SMARTBULB, even for led strips, power strips, and dimmers, so we may still need to pass extra type information somehow :-(

That’s very interesting I wasn’t aware of any legacy devices other than IOT.SMARTPLUG using the new discovery protocol. Would you mind pointing me to some issues etc so I can get the details?

[rytilahti]

So they are not using the new discovery protocol, but there's a type/mic_type key in the legacy discovery responses (just as in the usual sysinfo ones). The problem is that dimmers, led strips, and powerstrips do not have a separate type so there's that logic inside the discovery to construct corresponding device instances for them :-)

[sdb9696]

Some future work from #557 and #552 is captured here:

• Common Interfaces (and tests running on all devices): #557 (comment) #557 (comment) #557 (comment)
• Unpacking newfakes.py: #557 (comment)
• Error code checking and mapping: #557 (comment) #559 (comment)
• Missed removing TPLink prefix from TPLinkKlapTransportV2: #557 (comment)
• Better docstrings for the different protocols: #557 (comment)
• ConnectionParameters: #557 (comment)
• Consider passing keeping dicts for longer in the transports: #557 (comment)
• Update FakeTransport to allow set commands: #557 (comment)
• Add tests for consistent constructor parameters: #557 (comment)
• Add query logging (somehow dropped): #552 (comment)
• Support multiple requests: #552 (comment)
• Add more tests: #552 (comment)
• Improve re-use of protocol code, particularly around retry logic #552 (comment) #557 (comment)
• Simplify request id generator: #552 (comment)

Edit: All complete now except the request id generator moved to #777 and #778 opened to improve test coverage.

[rytilahti]

Feel free to create separate issues, at least for the issues that we should fix (or PRs to merge) prior cutting a new release. This way we can add a milestone to them to track the progress :-)

One more thing I realized is that we want to provide a way to use the CLI w/o discovery, too. This could be done as a part of the connection parameters PR to make it easy to test how the interface feels to work with.

[sdb9696]

One more thing I realized is that we want to provide a way to use the CLI w/o discovery, too. This could be done as a part of the connection parameters PR to make it easy to test how the interface feels to work with.

This is completed

[rytilahti]

Some more discussion points Originally posted by @sdb9696 in #569 (comment)

1. Do we want to take a similar approach for the discover() and discover_single() methods, i.e. consolidate the parameters into the same or similar class?
2. Should we drop the host, port & timeout variables in protocol classes and have getter properties that get them from the transport?
3. Same for devices. We currently have a lot of duplicate variables around.
4. Should we take this opportunity to drop the credentials from the devices and remove them from their constructors? I think it would be better for the device constructors to take the protocol instance as parameter otherwise we're just setting it right after construction every time.

[rytilahti]

1. In my eyes the discovery is a very special case and we should keep it as simple as possible, as you don't normally want to serialize that information (i.e., we want to avoid having to construct extra objects to keep the API as straightforward as possible.
2. It makes sense to have a single source of truth, which should be the connection parameters, right? Let's give that some thought later, but having some helper getter properties that access the relevant pieces in the connection parameters sounds like a good choice.
3. Agreed on devices, but this is also something that doesn't need to be figured out immediately. We want to provide a stable, usable API in the end after all.
4. Same as 3. I'm not sure what's the best approach to make the API nice, but I'm pretty sure we should not require users to figure out how to create a protocol instance and pass it into the ctor. One potential option could be to keep the credentials, and force discovery/create() if the initial update fails (i.e., the device is not using the legacy protocol). This way, it is only required to pass 1) the device host and 2) the credentials (not mandatory).

[sdb9696]

These questions are now closed,

1. Was decided not to do, 2. we dropped the duplicate variables and the device config is the single source, 3. as per 2, 4. we dropped the credentials from the device constructors.

[rytilahti]

Stumbled upon an interesting paper on the security of these devices, Smart Bulbs can be Hacked to Hack into your Household by Bonaventura et al., which provides an overview of the used protocols.

[sdb9696]

Yes I read this too. It made me wonder if we should put a warning for aes in HA.

[bdraco]

Yes I read this too. It made me wonder if we should put a warning for aes in HA.

We don't want to cross the line of giving security advice in HA as it usually doesn't go well and it's not really HA's place to do so as it comes off as nannying users.

It's further compounded by the fact they already have the hardware in hand and are likely to get defensive

[rytilahti]

Nah, not really an issue relevant to users IMO, given that it's all local. Otherwise most of the locally controlled integrations would have warnings about similar issues :-)

[sdb9696]

Closing this discussion now as all items addressed or moved to issues.