Add support for config flows for switches #6
Conversation
|
Super cool, thanks for writing this functionality. Sadly, I don't have any Tuya switches, so I can't test it. I think that the ability to edit settings after a device has been added is pretty important for a nontrivial amount of use cases. For example, someone might have their ID and Key, but not their dps information yet. They might want to start running localtuya with logging enabled to see the output of their dps values. At that point, they would not be able to edit their settings. I can't speak for switches, but for covers, there are several dps keys that go in the configuration. Perhaps we want that functionality added before this is merged? Or, at the very least, add a "warning" that informs the user that they will not be able to edit the configuration after adding a device, and would instead have to delete and re-add the device. Or, as an alternate approach, do we want to add something in the README explaining that you must already have your dps values before installing localtuya? Unfortunately, my familiarity with Home Assistant is not at the level where I would be able to write the functionality to edit the configuration after a device is set up. That said, once this pull is merged with localtuya, I would be happy to try adapting this for cover devices. |
|
Thanks, you should get some devices 😉 While I do agree that it would be good to support editing settings, I don't see it as a blocker. It's better to start small and extend. The solution I would like to see is querying all DPS parameters and expose those values (as dropdown lists). That way you don't have to go anywhere else. It's trivial to implement, assuming I can figure out how to query all DPS values and not just "the first one". I've tried to compare with tuyapi, but I can really spot what's wrong. Will have to look into that further. For now it would probably be best to just write something in the README. The process of setting everything up, e.g. getting local id and key, is quite tedious as-is. No one will be able to set the integration up via either config flow or YAML without resorting to the README anyways. |
Yes, I agree. Let's just put something in the README for now noting the limitations, and as more functionality is added, we can update the README to reflect the current install/configuration/usage instructions. How exactly are you going about trying to query all DPS values? I've been able to query my DPS values across multiple libraries and repositories. Can you query the DPS using the test.py debug script in the rospogrigio:master repo? Happy to help you debug this. |
At least with tuyacli, it's possible to get a raw dump of all DPS values from a device. Usually you pass a dictionary with the DPS:es you are interested in, but supposedly you should be able to send an empty dict and get all. You of course just get IDs and values without any real meaning, so you still need to decide what they mean. Some of them are quite simple to deduce though. Tried the test script and it worked for one type of the sockets I have, but not the other. Something fishy is going on there. For the one that worked I got all DPS values: |
Yup, with tuya-cli, you can add Let's call "Device A" the one that does work with the test.py script. A few questions: (1) Can you post the output of trying to dump Device B with tuya-cli using the following command: |
(1) It seems like when specifying protocol version it works as expected with tuya-cli: (2) It's 20. So I guess it's some kind of incompatibility in the pytuya library used here. I haven't looked at the protocol nor the implementation much, so I can't really tell. If you or someone else knows more and can help me fix this, it would be great! |
|
Here's a prototype (not done yet) of what can be done if DPS values can be fetched correctly: |
|
Interesting, my expectation was that the device ID of length 20 would be the one that doesn't work. rospogrigio helped me debug my cover device that wasn't working a few days ago. We discovered that rospogrigio then merged the change into the master. Anyways, you're telling me that your device with ID length 20 is working, and your device with ID length 22 is not working, so this is clearly not your issue. Again, I just want to provide some context for others who read through this thread. Let's go back to your problem. I've been in touch with @jasonacox, the author of tuyapower, a python library that polls WiFi Tuya compatible Smart Plugs/Switches/Lights. tuyapower depends on pytuya, just as localtuya does. pytuya is no longer maintained, so @jasonacox has forked it into tinytuya, which he says he plans on maintaining. He's already merged in the 'device20' change. Let's run a series of tests, only for Device B (the one that wasn't originally working with the localtuya debug scripts to pull dps data). This is going to be a bit duplicative, but I want to determine exactly where the error does and does not occur, and get as much information as possible. Test Series 1 Test (1)(a) Test (1)(b) Test Series 2 Test (2)(a) Test Series 3 Test (3)(a) Test (3)(b) Test Series 4 Test (4)(a) Test (4)(b) Test Series 5 Test (5)(a) Hopefully, this can help narrow down where the problem is (and isn't)... Worst case, if none of this works for your device, maybe we just add tuya-cli as a dependency/import it (whatever the correct terminology is)? It seems to be the most consistent method to get dps data. |
|
Yeah, I read something about the different lengths earlier. But as you say, I don't think it has to do with that. Or at least not directly. Test Series 1 The general behavior is that the device doesn't respond to requests at all, like this: Scanning reveals the device correctly: An interesting thing though... I tried removing the "dps" key from json_payload altogether and in that case I do get a response: I guess the format is not what pytuya expects, so it doesn't work. Test Series 2 Just get repeating timeouts. The exception below is printed when pressing ctrl+c. I tried modifying that line to set an empty dict ( Test Series 3 Will have to do this later as I'm in the middle of stuff. Test Series 4 Scanning: Output from The script hangs until i press ctrl+c, which prints the output above. Values doesn't seem right, so I don't think DPS values were retrieved correctly. Test Series 5 Based on all of this it seems like excluding the dps field and handling is what works. This however does not seem to work with my other ( |
|
Guys, I would make it more simple. As far as I understoodk, "device22" devices require the list of dps you want to get in the response to be specified in the payload of the request. I would simply send a list of, let's say, all numbers from 1 to 25 plus from 101 to 120, and we would probably get all the dps involved. |
|
@rospogrigio That sounds like a very hacky-last-way-out kinda thing to do. It tuyapi can do it without providing a list of DPS values, we should be able to replicate that. Moving to a library that is maintained sounds like a very good idea though 👍 I'm making a few big changes to this PR, moving everything around a bit. The idea is to have a "generic part" used to connect to the device (e.g. key and id). After that entities can be added based on the supported platforms. This basically means that you can create any kind of entity and attach it to any tuya device. We could for instance create a |
|
@postlund , you seem to be quite more skilled than I am... would you like to take over this repo and become the maintainer? I don't know if I can add other developers but I believe so, you also seem to have much more time to spend :-D |
|
Moreover, I gave a look at tinytuya... it is substantially the same exact code of pytuya, with some commented debug instructions removed BUT without something that in my experience is fundamental! I am talking about a second receive that has to be issued in certain cases: some devices in some cases respond with an empty message (tipically 28 bytes long), and when this happens you have to wait some time (I use 100ms) and try another receive call, which finally returns the correct data. |
|
Let's see where this goes first, but sure, if you want to get rid of the repo I can probably take over. Time is something I don't really have, but kinda take anyways... 😉 Do we know why the devices sometime respond with that message? Or what that message means? There are a few things in the code, mainly the integration, that needs to change over time. One is that updating is done per entity instead of per device. This means that when you have a double gang socket for instance, two requests are made but only one is really needed. This can likely cause problems as only one TCP connection can be active to a device at the time. The second issue I know of is kinda related as well. There's no synchronization with regards to only establishing one connection at the time. So if an update is performed at the same time as doing something else (turning of a switch for instance), we again get two connections at the same time. Turning on and off rapidly surely causes problems as well. This is somewhat mitigated via the "cache" (which is something that isn't really needed either as everything is suppose to be cached in the entity) since it performs retries. Using a manager that keeps track of the connections and only allowing one at the time would be better. Over time I will try to fix these things as clean up the code. Cleaning up the code will likely be the first thing I do after getting this PR merged. |
|
What message? The one that requests a double receive you mean? It's actually an empty message (once decoded, you get a
OK, let's keep this contributor-and-maintainer approach for the moment, then you might take over the project when you'll want. Or, I could add you to the developers, when I figure out how to to it (maybe you know). |
|
PS, I also imagined that having 2 different devices for the same 2-gang switch was something that could be optimized, but would not know how to do it. |
|
Sounds messy, I might have to take a look at that later. But it probably works good enough for now as-is 👍 Once cover has config flow support you will be able to add that configuration. But it will not solve the double-connection issue, it's a different a different problem. I will fix that as well though. PS. You should probably enable issue in this repo, so people can report bugs. |
|
Mmm... how do I enable issues? |
|
Click Settings at the top of the page and check under Features. Go to Configuration -> Integrations, press the +-sign down to the right and look for "LocalTuya integration". You should be able to follow from there. |
Thanks @rospogrigio - You are correct, the Tuya devices will often behave this way. There are two schools of thought on this:
Having said that, it occurs to me there is a 3rd option. I can add a setting to disable retry if the client code wants "true response" behavior but default with adding the retry logic. I'll look at your retry logic to see if that makes the most sense. I'll also check TuyaAPI to see if there are logic benefits we can glean from there as well. |
|
@andrewmeepos I did an override on the device type and forced it to use device20 despite being a device22 and now it works. I also compared to what tuyapi does it it's the same thing. |
|
Can anyone tell me or point to the source of the "device20 vs device22"-thing? Doesn't seem to be a special case in tuyapi, so I'm suspecting there's something 🐠 going on. |
I also just went through all of tuyapi's codebase and don't see any delineation between devices with IDs of varying lengths (20 vs 22, or otherwise), nor any mention of hexbytes of 0a vs 0d. Perhaps tuyapi handles device communication in a way that negates the need to differentiate between devices with different ID lengths?
I support this option. Would be helpful going forward if we could generally be in line with tinytuya. |
|
@jasonacox I see, the fact is that some of my devices (the covers, if I remember well) happened to enter some kind of state in which there was no way to get a non-empty response using a single receive. Making a second receive made them work flawlessly. I don't know if it was a matter of firmware, just today I noticed that a fw upgrade was released and I installed it, I well check if this behavior has been fixed. |
|
@postlund I don't find any Features page under Settings... |
Interesting, let us know the result after you update the firmware. For everyone's reference, the original commit to TradeFace's python-tuya library that incorporated the 0a/0d change is located here: It looks like originally, there was no specific connection between 0a/0d and device20/device22. I think it could be a coincidence that @rospogrigio 's device20 devices needed 0a as the first hexByte, and his device22 devices needed 0d as the first hexByte. Because, as @postlund just told us, he has a device22, and he had to override it to be a device20 to get localtuya to work. That seems to be a concrete example of a device22 needing 0a as opposed to 0d. Regardless of whether there is a connection between deviceID and 0a/0d, one thing seems clear to me, which is that however tuya-cli/tuyapi approaches this seems to the most robust method, as they don't seem to delineate based on device id length and it sounds like everyone can successfully communicate with their devices using those libraries.
Here's where the option to enable issues is located for my fork. Let me know if this helps you find it, or if it really is missing. |
|
Nice discovery.
@rospogrigio Did you find that that TuyAPI worked with your device22 devices? I'm cleaning up |
|
So, sorry for the long post I'm about to write.
|
|
@jasonacox I agree with you, it seems really strange that One approach would be to provide a simple interface that tries both types and returns whichever works. Since it's static for the device, it can be cached in a configuration (e.g. the config entry with regards to this PR). |
|
One thing that is really missing is to be able to fetch more metadata from the device. For instance firmware version, product name, maybe manufacturer. Not sure if that is possible but would be really good for device support. |
|
I hardly doubt so. BTW, I tried launching the scan.py script in tuyadebug, and it recognized all of my devices, regardless if they were type 0a or 0d. This is done by using UDP connections, and it returns these info so that they could be autodetected, without the need for the user to input them (in particular, device_id and protocol version: I need to study a bit more how things work before I can be helpful in this process, for the moment I am comparing what you are doing with what daikin does, to understand what we can borrow from them. In the meantime, if I run your RP I find your entities under Configuration, but they are all unavailable for some reason. |
|
Yeah, discovery has been in my plan all along. Biggest issue is that Home Assistant doesn't have an api/allow custom discovery schemes. So we can't integrate it with the default discovery integration as only zeroconf and ssdp are supported. I was thinking we could add it as an initial step in the config flow and pre-fill discovered information. We need to manually register a device in the device registry and implement |
|
Regarding the entities being unavailable, maybe you can debug what happens in the |
|
Yeah, I believe the easiest thing would be to have the user input the IP address, and have devID and protocol autodetected in this way. I was also having a look at what the Tuya integration does, and I am pretty disappointed to see that they have just made an entity for each subswitch (without regrouping them for physical devices), and they don't even provide the voltage/current data... so we are aiming to code something better than it is done there. |
|
We can do even better and just detect all devices and present them in a dropdown menu. Will make it super easy to use. But we need some kind of fail-safe in case discovery doesn't work. It's probably the least amount of work to get something up and running. Integrations usually evolve over time. But yeah, we will be aiming for a higher goal 😊 |
|
OK I finished my time for today, will try something tomorrow... if you have something new to push in the meantime, please do. |
|
@postlund , I've been studying a bit how things work, and I am getting more into the situation. I am planning a big refactoring of pytuya library since I really don't understand why there is a 3-level class inheritance, while I believe 1 is more than enough. Moreover, class specialization is partly done in the components script and part in the library, which has no sense because the library should just provide generic device handling. EDIT: done. I strongly suggest you to merge from the "pytuya_refactoring" branch I pushed, I made an important cleanup of pytuya and things are much clearer now. Now I'll proceed with the device-subswitches thingy. |
|
OK, I think I'm starting to understand how things work and I've accomplished something. In my branch "physical_device", you can find a merge of your PR with my refactored pytuya. Moreover, I was able to create the Device (under Configuration-Devices) which has the entities associated. It works both for config flow and for YAML files, even if I have to think which is the best way to handle the "names" and "friendly_names" (need a discussion here). Next steps:
|
|
Sounds like you are making good progress too 😊 One thing that is important now is that we are structured, because there are a lot of work ongoing now that depends on each other. So we need to make sure that things are possible to merge without too much hassle. Your stuff, the pytuya rewrite and device registry stuff would be great if you made into two separate PRs. I can review them as a second opinion. Once they are merged I can start rebasing my PRs on top. I'm also working on support for options, so that will come soon. I believe that we should go even further with pytuya and implement proper support for the protocol. By doing so we can maintain a connection to the device at all time, instead of connecting, doing something and immediately disconnecting (which is not a good approach). I would also strongly suggest that we convey to an asyncio interface instead, so we can convert the integration to be fully async. The cache-stuff that exists today should be possible to remove, we might wanna keep some retry-logic somewhere. But it should be re-usable. We can add a generic |
|
Yeah, sorry for not using 2 different branches. My suggestion now is that you merge with my latest work ("physical_device"), so that we have a common base to start on, and then continue the work from there. Then we need to decide who does what in order to avoid wasting time and energy. |
|
My recommendations based on experience: Never push or merge to Use branch permissions and lock pushing to Make small changes and push them as PRs. Stick to one topic and change one thing per PR. Rationale: It's for instance better to make one PR with refactoring and another one with the actual change, in case refactoring/restructuring is needed. Again, it's easier to review and history ("blame") makes more sense. Avoid feature branches and large merge Integrations. Avoid merge commits altogether and force rebasing. I could probably add a few more things, but what I'm after is basically: lock down |
|
I believe it's gonna be quite a challenge to sort this out TBH. |
Let me know if there's anything I can do to help move things along. |
|
Thank you for the precious suggestions @postlund . I didn't know I could create a PR on my own repo, good to know. I'll try to do what you suggested, in the end I made very specific changes so it should not take that long. Also I have just noticed all your other PR, so I"ll merge everything and we can then restart from there.
I think I'll release the autodetect part in the meantime, and in the next day I'll proceed with the mergings. |
|
Yeah, you can submit PRs to any branch in any repo (even your own), which is nice 😊 Maybe we can do it like this. You extract your changes into a PR or two, we review them and merge. Then you make a release. After that we start a new "development cycle" in order to release something along the lines of 1.0. There are a few breaking changes I would like to make in the YAMl config, I.e. remove fields which are not used. So it's probably better that we try to get as much stuff in as possible to figure out all the breaking changes we need/want to make to not leave any technical debt. Does that sound like a decent plan? With "one track" I mean that there's only one branch to work on, I.e. Rebasing is how you keep up-to-date with another branch. You basically change the "base" of your branch to another branch, applying all your commits to the new base. When applying this to a PR, it basically means that instead of taking all commits in your branch and squashing them together into one merge it, all commits will be applied on top of the branch you are submitting your PR to (e.g. |
|
Did did a review of #11, left some comments. Overall a well needed change! #12 is too big for me to review and too much noise from #11, so I'm gonna miss stuff. It would be a lot better if you removed the stuff from my PR and kept the device support only, so I don't have to review my own stuff. Once #11 is merged and #12 is rebased, I can check again. |
|
Well, looks like the multiple merge broke almost everything... anybody can help me debug what's happening in master?? |
|
Yeah, I was kinda hoping we could take one at the time and make sure it worked before merging. I can take a look but not until tonight. |
Add question regarding blocked internet access
…nection Using single gateway connection
Fixed initialization with API but no internet.
This PR adds initial support for config flows, but only for switches at this point. We should start with one platform and create one PR per platform, which should be pretty easy after this.
Summary of what is supported and not:
iconwas removed as it can be overriden as a customization and it also didn't work with more than one subswitchFeel free to give it a spin and give me some feedback. I have tried it with my single and double gang switches and it works fine.