-
-
Notifications
You must be signed in to change notification settings - Fork 346
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 schema functionality #164
Conversation
The idea is great. So people will be able to get the state and turn things on and off without having to figure out what dps to use. I had a quick look at the code and there are quite a few if statements there to deal with the changes. It might be nicer if the new features were split into a new function. I think in the other thread there was a suggestion of a new function so maybe something like get() for the new translation bits which calls get_raw() with the current get code, or something of that like. You also have the connect function running a plain get for say dps 1. I've never really liked that since in your example above it runs a get which is ignored. And in my code I have weird if statements checking to see if they want just a plain get or actually want another dps, etc. So maybe change the get() in the connect() function to a get(schema) and set that as the available schema. So you don't need to define it on init. I only have plain devices so I have limited experience with these sorts of setting. I probably should buy some cheap devices with multiple dps options and settings. |
I'm with you here, I think we can minimize the branching through some layered abstractions rather than a single interface for high level and low level interactions. A developer using the framework could then decide which layer of abstraction is right for their application.
Same concern and solution as above.
I like this idea, but I don't think it should be the default (another layer?). We'll have a little work to do in order to automatically deduce the meaning of dp values, maybe through some sort of dps signature database, and even then we won't be able to cover every device. I have a partial solution based on this idea implemented in mocktuyacloud. |
I also wanted to mention I'm happy to see utility functions moved out of the TuyaDevice class. I think this is heading in the right direction and that we can do better still 👍 |
Oh, I thought the devices sent back the names/types of of the dps. But it seems like you need to define them. It seems like we are trying to move more towards how homassistant tuyapy api works and is set up. https://pypi.org/project/tuyapy/. It uses a custom tuya endpoint https://px1.tuyaeu.com/homeassistant but seems to use a custom api. That is set up all around devices and there isn't a way to directly set the dps. So there may already been some kind of mapping or database already set up, so when you tell the api to switchDirection, or setWindspeed all the dsp stuff is done by the api. I ported it to js, https://github.com/unparagoned/cloudtuya/blob/dev/cloudtuya.js and initially wanted to merge with tuyapi or tuyapi/cloud. They just seem to work completely differently and so there is effectively no cross over. |
Was thinking about removing this. But it's not really ignored; the purpose of it is to emit a data event upon connect so the user can get the current state as soon as possible automatically.
I wanted to make it backwards-compatible; if you guys think that'd it be better to just introduce a completely new interface in v5.0.0 that would be stable for the next few major versions that's fine too. |
There are a couple options for deriving this information. I'm still looking into it.
Interesting, I didn't know about tuyapy, thanks for sharing. Sounds like it uses a similar API as Tuya's openApi. We could potentially utilize this to build up our own database, as personally I want to avoid using cloud services, especially for this project.
Similar story with mocktuyacloud; initially started making changes to tuyapi to contribute upstream but the scope quickly changed. Now I'm going back over it to see which parts could be applied to tuyapi without changing the scope of the project. The primary difference with mocktuyacloud is the ability to sandbox the devices, in addition to having complete local control. You can communicate with the device via LAN, MQTT, and HTTP. You can perform all functions normally served by the cloud including initializing the device and issuing keys without Tuya involvement whatsoever. As it stands, while tuyapi enables LAN control, it does not prevent information flow to and from Tuya's cloud via MQTT and HTTP. In theory a malicious actor could gather information from your device, compromise your device's control, or even go so far as to replace the device firmware entirely, compromising your internal network or just straight up bricking your smart devices.
I think we have a little work to do in strictly defining what tuyapi will and won't do. We should balance scope bloat (I'm bad at this) with providing enough options for developers to build off of. I've been careful to not change the scope so far; I think adding schemas does widen the scope but still could be layered in such a way as to not break (more or less) the existing model. I think it would be very helpful to put together a formal class diagram (another thing I'm bad at) and use that to structure v5.0.0. We're getting to the point where a lot of new (and good!) ideas are flowing in, and coming to an agreement on the high level structure will help us introduce these without breaking stuff and without a ton of runtime checks for this situation or that. |
A rough idea of the breakdown in mind
|
If we wish to expand the scope to include some or all function of mocktuyacloud, perhaps
|
Expanding scope once again, it would be easy to see how @unparagoned's TuyaCloud control fits in under |
A lot to consider. It's very easy to bloat scope as it's fun to explore every imaginable mechanism this framework could offer. It's important to stay within our means as maintainers and also to consider the unix philosophy of having a program do one thing and do it well, and let the user string those building blocks together. |
Anyway, my point (since I've seemed to gotten a bit derailed) is that keeping a layered approach such as the ones described above allows the developer to choose how much abstraction they want and keeps surface area of the API available to build new components on. |
Some comments on feature scope:
I agree, not planning to ever add cloud control directly to TuyAPI. I'm happy to accept PRs for @tuyapi/cloud to better support Tuya's Open API (didn't know that was a thing, was it recently added?) @unparagoned but it's fine if you want to go your own way. (I could also make a new repo in the @tuyaPi org for your project and add you as a contributor if you want.)
True, but I'm not planning on adding a DNS resolver to TuyAPI anytime soon 😛. This is probably a problem best solved with other tools, although it'd be cool to make a guide on how to do it.
While it's tempting to build abstractions directly for devices, like a RGB bulb, or maintain a database of said abstractions; I think that's a task best left once again to other tools. Personally, the highest level of abstraction I would be comfortable with is what's roughly contained in this PR, where the user defines the abstraction themselves. That being said; it might be really cool to build a second library on top of TuyAPI that did those type of device-specific abstractions.
This component should probably be separate as well, similar to @tuyapi/link In the end, I guess the Unix/Node.JS/my philosophy could be described as "make decoupled components that work well with or without each other, and allow the user to work at any level they desire". |
Sounds good, thanks for clarifying your intended scope.
I probably did a bad job at explaining the intent of these classes in this organization. I don't mean to create TuyaRGBW, TuyaSocket, etc... (but I do agree another library that depends on tuyapi would be great for this). I envisioned
I'd be happy to work on tuyapi/link, as well as any other tuyapi/*, to build up these features, while remaining decoupled from each other 😄 I've got a fairly comprehensive implementation of a fake device a la tuyapi/stub that I can contribute too.
I'm totally on board here! Thanks for clarifying. Alright so I think we're in a good place; we'll work on a class breakdown for v5.0.0, and in the interim clean up v4 as best we can without breaking changes. Sound about right? |
I have various thoughts on this. I think my preference would be to make connect a private function so no-one can call it and have get/set call it it, when they are run. If it's already connected it doesn't do anything, but other wise it will connect first then perform the action. I don't really understand the point of manually connecting. My main issue is that the behaviour of connect at the moment is that it is different than a get, you need to access that data in a specific way, then it only works for some use cases. If someone wants to get dps 2, then that get call in connect is is a complete waste. And then if I'm just using awaits like in your code it's missed even if it is required. So from the code point of view it's best if I ignore that fact connect calls get and write nice clean code. But from a performance point of view I have double the code length to do for if statement to see if I cant use that automated get call. So my initial view is that I I'd rather it go. It's kind of strange behaviour that requires reading the source code(or README) but isn't that natural. Alternatively for backwards connectivity just have connect returning the get, with all the dsp settings if it can be used. That way connect acts the same as usual but also does a bit more to make it more consistent with how you would expect a get to work. Maybe by renaming connect to raw_connect, have connect as an alias to get, and have get calling raw_connect. So connect and get act the same, if you need to connect you connect, and both return the state. That adds back the backwards compatibility to v3 in how get worked. Currently get() has lost the previous ability to connect() so you need to add that to your code. And connect has this weird half version of get. Either make them completely different or just make them do exactly the same thing. Making connect kind of but not properly doing a get it's just messy.
Sorry I probably mixed the function names up but I was trying to suggest a completely backwards compatible setup. So if you enter the original content, get would just pass it to get_raw which was the original function.
I didn't want to go my own way, and if you look at some of earlier commits and code you'll see I was trying to add functionality to tuyapi/cloud. But like I mentioned it seemed like the api tuyapy used was completely different from the normal tuya api used by tuyapi/cloud. Even the non eu endpoint regions were completely different. So while I've pretty much written a clone of tuyapy, lots of stuff is hidden behind that separate api interface so I don't really know how it works. I wasn't raising a point about integrating cloud, just pointing out that it feels like what that specific api seems like it does what we want. I'm not sure who wrote tuyapy, it seems like maybe even Tuya wrote it for HA, but maybe it can be used to get or figure out the schemas for all tuya devices rather than starting from scratch ourselves. So it was mainly a suggestion for creating a database for the schemas for local control only. But I have no issue about moving cloudtuya to @tuyaPi. I think the dev branch has all the features from tuyapy. In terms of coding is probably just doing some unit test, testing some of the token renewals, testing non-standard features like fan/temperature/control and other regions, country codes and apps. Also I think there was probably a good comment in other thread about scope. @codetheweb you have various tools and I've only just recently learned they were your tools. So It would be useful to have some kind of map or diagram to see the scope of each tool, where does tuyapi start and end, and where does tuyapi-cli start, etc. |
Sounds like a plan @kueblc.
This was exactly v3's behavior @unparagoned. I changed it when we removed the non-persistent connection option, as I didn't think that the code should implicitly call
That actually makes a lot of sense and clarifies my thought process as well. I'm in board with removing the
I'm pretty sure someone from HA actually said they did. It's a bit of an interesting move for them, as they don't offer any other official libraries or even decent documentation.
Good idea, maybe there is a way to iterate over all schemas or something through their API.
What do you think about adding a section in the README of TuyAPI named "Officially Maintained Projects" or something similar; with a short description of everything under |
Closing this because #169 (comment). Thanks for the good discussion. |
This isn't ready to be merged yet, just wanted to get the conversation going. I added the schema abstraction layer as discussed here. I think this is a really elegant way to flexibly extend TuyAPI's functionality, depending on the use case (thanks for the idea @kueblc).
Here's some sample code:
The
transform()
function defined in the schema allows the return value of get requests to be easily manipulated. I'd like to extend this to set requests as well, but I'm not sure if there's a more elegant way to do it other than just adding a second transform function.Pinging @unparagoned and @kueblc for feedback.
Todo: