Skip to content

Conversation

@sdb9696
Copy link
Collaborator

@sdb9696 sdb9696 commented Dec 29, 2023

This PR adds a helper class for making all known requests to new smart protocol devices with their respective request parameters. Currently it includes changes to dump_devinfo and new fixtures but this can be separated into different PRs before merge. For now I wanted to give the wider picture of all the data we get back for each request etc.

@rytilahti
Copy link
Member

Just some quick thoughts below.

So we will definitely need to split this up, . One potential approach would be by creating a module-style structure per nego responses that will contain the individual methods and getters into clean, logically separated "modules". Basically sharing the same idea that's used by the "legacy kasa" at the moment.

Although, maybe we should slow down a bit and instead add these queries at first only into the dump_devinfo? This way we could start already collecting fixtures with extra information for future uses.

IMO, a great next step before going deeper into adding more features would be to start thinking about introducing some type of introspectable interfaces for device classes. Basically, what we will want in the end is a way to inform our cli tool, home assistant integration, and other downstreams about available actions, sensors, etc. without having a need to hardcode any of this.

The end goal would be to necessitate only changes into the library code, releases, and version bumps to introduce support for new features :-)

But first, we should probably cut a new release with what we already have, get that home assistant PR in, and then continue with further improvements. I suppose I want to ask what do you think we should still get in prior to making the new major release? You had some fixes to the bulb support, would you prefer to have those already in?

@sdb9696
Copy link
Collaborator Author

sdb9696 commented Dec 30, 2023

Just some quick thoughts below.

So we will definitely need to split this up, . One potential approach would be by creating a module-style structure per nego responses that will contain the individual methods and getters into clean, logically separated "modules". Basically sharing the same idea that's used by the "legacy kasa" at the moment.

I agree we should think about how we structure the functionality and "modules" seems like the best approach. However I do think it might make sense to keep all the SMART queries in one file and call them from the modules. Firstly because some of the parameter classes and requests are shared between modules, i.e. GetRulesParams and any modules that set values via set_device_info, and secondly because it's handy to have one place where we've captured everything we know about talking to the devices.

Although, maybe we should slow down a bit and instead add these queries at first only into the dump_devinfo? This way we could start already collecting fixtures with extra information for future uses.

I agree and I've created a PR just so we can start collecting the fuller fixture information (N.B. I think we have nearly all the info we'll ever want). We could always break the smartrequests.py file up later if we want but as dump_devinfo is the only user I hope it doesn't matter at this stage.

IMO, a great next step before going deeper into adding more features would be to start thinking about introducing some type of introspectable interfaces for device classes. Basically, what we will want in the end is a way to inform our cli tool, home assistant integration, and other downstreams about available actions, sensors, etc. without having a need to hardcode any of this.

The end goal would be to necessitate only changes into the library code, releases, and version bumps to introduce support for new features :-)

Agreed

But first, we should probably cut a new release with what we already have, get that home assistant PR in, and then continue with further improvements. I suppose I want to ask what do you think we should still get in prior to making the new major release? You had some fixes to the bulb support, would you prefer to have those already in?

I agree with you and I think we should draw the line here, cut a new release and get the HA PR finalised. Either with or without the dump_devinfo PR, that's up to you. We can start adding functionality afterwards but for now at least we will have fixed any unsupported kasa devices and provided basic TAPO support.

@sdb9696 sdb9696 marked this pull request as draft December 30, 2023 11:47
@sdb9696
Copy link
Collaborator Author

sdb9696 commented Jan 30, 2024

Closing this as mostly superseded now by other PRs. The logic in conftest to consolidate the fixture warnings could be useful so I'll leave the branch open.

@sdb9696 sdb9696 closed this Jan 30, 2024
@sdb9696 sdb9696 deleted the add_smart_requests branch March 7, 2024 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants