Refactor and Clean Up

[codetheweb]

Done quite a bit of work over the last few days attempting to slim down TuyAPI and make it more readable. It's now much cleaner, although at the cost of potentially breaking older code.

Major Changes

• Persistent connection is now default. This has no real downside as long as the user calls .disconnect() when they're done.
• Both .get() and .set() will return a Promise while also emitting an event with returned data. Again, I can't think of any downside to doing both at once instead of one or the other - and it greatly simplifies the code as well as function arguments.
• Converted TuyaCipher to an ES6 class (shouldn't change much, but looks nicer).
• Renamed .resolveId() => .find(), as the function can now look for both a missing ID and IP.

Those are the major changes, although there's some smaller ones as well with updating documentation, checking function arguments, etc.

As this is a breaking change, it'll be v4 when released.

Testing

With all that said, it could still use testing. Specifically, @neojski could you test finding multiple IPs? My college's network isn't kind to UDP broadcasts.

In general, though, just want to have anyone who is able to do some testing of these changes before I roll them out.

Mentioning a few possible beta testers:

• @unparagoned
• @jepsonrob
• @tjfontaine
• @NorthernMan54

(Fixes #139.)

[coveralls]

Pull Request Test Coverage Report for Build 213

• 54 of 75 (72.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-2.4%) to 68.182%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/cipher.js	13	22	59.09%
lib/message-parser.js	41	53	77.36%

Totals
Change from base Build 197:	-2.4%
Covered Lines:	63
Relevant Lines:	84

---

💛 - Coveralls

[unparagoned]

All I had to do was add disconnect to my script and it seemed to work fine. I didn't have to add any calls to connect. Which means when I call get(), tuyapi connects and gets the state and then runs get a second time. Your toggle function does the same. There didn't seem to be a nice way to change the behaviour because you'll then have to pass options to connect, etc.

persistentConnection: false
I then tried setting persistentConnection to false, it hangs if there are no disconnects. Apart from being mentioned as an option there is no logic to make any difference. You probably best off getting rid of it, but if not the following seems to get to let it work with my script like normal. unparagoned@64a116c

[codetheweb]

Thanks for pointing that out, I thought I left logic in to disable the heartbeat packets if persistent connection was set to false; but I guess not. Probably does makes sense just to remove that option entirely.

Right, .connect() is meant to be called multiple times in the same script. If a socket is already open, the function just exits and doesn't do anything. Does that sound right?

[unparagoned]

Yep I see how connect works and how if it's already been called and a connection is already open it won't do anything but I in my scripts I don't need to call connect I can skip that and go straight to calling get and set which will call connect anyway. But the way it's written seems like it's designed to run connect and might be an unnecessary step. If I just want to set the state quickly I don't want tuyapi to first get the state before it tries to set it like I wanted. Also when I get a state it gets it and then gets it again.

Also I ported over those those feature on my branch I mentioned. So find gets all devices on the network. findDevices finds all the devices and gets their states. So in my script if you do the following, it gets all the devices and their states even without any keys.

let tuya = new TuyaDevice({
    key: "1000000000000000",
    persistentConnection: false,
});
tuya.findDevices();
console.log(JSON.stringify(tuya.devices));

https://github.com/unparagoned/tuyapi/tree/feature-cleanup

[codetheweb]

Yeah, I realize now that the .connect() in both .set() and .get() is an artifact of using a non-persistent connection - sorry about that, just wasn't thinking before. It does make sense to remove it if there's not a persistent connection option.

That .findDevices() is pretty cool in that it returns the state of every available device, but I'm not really seeing the use case for it. And if .find() returns an array of objects of device IDs and IPs, I think it would be fairly trivial for a user to write something similar.

Todo:

• Remove persistantConnection option
• Remove .connect() in get/set
• Add option to .find() to return all found devices

[NorthernMan54]

@codetheweb Max, I though I should let you know that I had refreshed all my Tuya devices with Sonoff-Tasmota last month, so no longer have the ability to do a regression. ;-(

[codetheweb]

@NorthernMan54 alright, good to know. I keep meaning to flash one of my plugs but I'm scared I'll brick it.

[codetheweb]

@unparagoned take a look, see what you think.

I can't test .find() myself because of my college's network.

Guess I could just make a temporary AP, might try that later today.

[neojski]

@codetheweb, I'm out of the country for 2 weeks. The earliest I could set multiple devices support is March 2nd.

[codetheweb]

@neojski alright, I'll try testing it myself.

[unparagoned]

@codetheweb the find function works pretty well. Well it works when I use it properly, I had been trying to use find after running connect. You could get find to call disconnect before running, but I'm not sure if those sorts of things just bulk up the code.
There is a minor issue around the fact it keeps on running until the timeout and during that time it keeps on adding devices every time it sees one, so there are a few duplicates in the foundDevices array. So you probably should checking to see if a device is in the array before adding it again. I used an object so I could do, if id in foundDevices, since it seemed more compact than writing a loop to go through the array.. (But I think there's probably some fancy way to easily checking an array without writing a loop using maps or something).
In my original branch I just waited until I saw the same id again and exited. But I think different devices send out pings at different rates so if you have a fast and slow device that might not catch them all.
I had a little play and kind of carried away, so I added an option to say how many times you wanted to see devices again and it would then return early rather than waiting for the timeout. It also throws out warning if the timeout occurs before it's seen a device twice since it's unlikely that it's cut off exactly as every device has been seen only once.
I think most of what I did just adds lots of bulk and kind of makes it more a mess for limited functionality. So you should probably ignore everything I said apart from filtering out duplicates from being added to the foundDevices list.
unparagoned@0239590

I've been playing about with the persistent connection it seems to be working fine. I'm still not sure why connect() calls get().
I couldn't figure out how to get the state of that initial get, so I would need to call get again. I also just commented it out since I sometimes got errors and the logs make it look like they are trying to run the gets over the top of each other. It looks like that connect sends it's promises and stuff once it's connected and doesn't wait for the response form the get it called.

 TuyAPI Socket connected. +xxx
  TuyAPI GET Payload: +xxx
  TuyAPI { gwId: xxx, devId: xxx } +xxx
get state
  TuyAPI GET Payload: +5ms
  TuyAPI { gwId: xxx, devId: xxx } +xxx
  TuyAPI Error event from socket. xxx.xxx.2.xxx { Error: write EPIPE
    at _errnoException (util.js:xxx:xxx)
    at WriteWrap.afterWrite [as oncomplete] (net.js:88xxx:xxx) code: EPIPE, errno: EPIPE, syscall: write } +xxx
events.js:xxx
      throw er; // Unhandled error event

Snippet from the calling code

    await tuya.connect();
    if (isCommand("Toggle")) {
        let status = await getState();
        dprint('Status: ' + status);
        await setState(!status);

Multiple devices/IPs
I've got a couple devices. I've always just send commands to each one separately, but so if you can let me know what exactly you want me to do with some sample code I'll give it a go.

[codetheweb]

.find() should be fixed now for returning all devices.

The idea behind the .get() in .connect() is that TuyAPI will automatically emit the current state as a data event once it's connected. You should be able to catch it by listening for the data event, like what's shown in the README.

Do you think I should leave that in, or delete it?

[unparagoned]

@codetheweb Do you expect us to read the README....but yeh it makes sense now, leave it in. Ignore me, I've been coming at this from the wrong angle of a synchronous/non-persistence setup. I spent ages looking to see if there was some kind of return or promise with that initial get state without much luck. I always usually just gloss over the asynchronous bit in the readme since it always looked a bit too complicated, but I can now see why it' a much better way of setting things up. I now know how to get that initial get state and fix any issues. Thanks

[codetheweb]

Sometimes the synchronous calls are the better way to do it, it just depends on what you're trying to do.

I'll merge and publish this as soon as I get around to testing .find() with multiple devices (hopefully later today).

[codetheweb]

Just tested .find() with multiple devices, it appears everything works after a small fix.

[codetheweb]

v4.0.0 has been published! 🎉

Please open a new issue for any bug reports related to the update.