Skip to content
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

How to structure Find? #34

Open
neojski opened this issue Dec 20, 2019 · 7 comments
Open

How to structure Find? #34

neojski opened this issue Dec 20, 2019 · 7 comments

Comments

@neojski
Copy link
Contributor

neojski commented Dec 20, 2019

JSON seems so have trailing garbage:

{"ip":"192.168.0.45","gwId":"12747566807d3a493eba","active":2,"ability":0,"mode":0,"encrypt":true,"productKey":"UhLkQlpfvO0fdrf3","version":"3.1"}VL'B

The VL'B above is the garbage. Find in https://github.com/codetheweb/tuyapi used to work fine. Also, it was way easier to use: here you have to subscribe to some updates and match them against your id rather than just shove the id into constructor.

@neojski
Copy link
Contributor Author

neojski commented Dec 20, 2019

Another issue with find is that it binds to 6666. So you can't run it twice at the same time.

@neojski
Copy link
Contributor Author

neojski commented Dec 20, 2019

Another thing to mention is that we could type Find better: right now broadcast payload is not typed at all.

@neojski
Copy link
Contributor Author

neojski commented Dec 21, 2019

I suspect it got broken here: 22d63a7#diff-dd2da9c147c9ed303950569858336545R66

@neojski
Copy link
Contributor Author

neojski commented Dec 21, 2019

Actually, that change broke everything: updating the state doesn't work anymore either.

@codetheweb
Copy link
Member

Thanks for all the contributions so far, forgive me if it takes a bit to go through them all 😛.

The find() method also bound to 6666 in TuyAPI. Having it be a separate class in this package was an intentional choice, because if you wanted to 'discover' multiple devices at the same time with TuyAPI you would have either had to (a) resolve them sequentially (takes a while) or (b) called find() with parameter {all: true} or something to return all received device packets (kind of breaks OOP and you still have to implement custom logic to update your TuyAPI instances to the correct IP).

Having said that, I think adding a helper method to the Device class that calls the Find class internally could make sense.

@codetheweb codetheweb changed the title Find doesn't work How to structure Find? Dec 21, 2019
@neojski
Copy link
Contributor Author

neojski commented Jan 4, 2020

So, I was thinking a bit as how persistent the connection should be implemented here. The most unusual scenario that I can think of is where the device is turned off and then back on but the router decides to assign it a different ip. To cover this scenario properly one has to run find before every reconnect. The main question, I suppose, is whether the persistent connection module is going to be part of Device or not.

Regarding find: I think I don't actually mind to make it a singleton because, in fact, it is a singleton: it has to bind to port 6666 so you can have only one instance of it at any given time (SO_REUSEPORT is not supported by node because it's not portable).

@codetheweb
Copy link
Member

Running find before every reconnect seems a little excessive, but I guess it makes sense.

I've thought about this the last few days and I think we should keep the reconnection (finding IP after device disconnects, handling exponentially backing off retries, etc.) logic out of this package, it's probably a better fit for the Devices library that will be written at some point. This should handle heartbeat packets and nothing more, as I've seen enough different approaches to reconnection logic by tuyapi users that it doesn't make sense to include it in the core package.

A singleton would be nice, but I don't have much experience with them and I've heard that they're a little finicky in Node.js. If it works, that sounds like a good solution.

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

No branches or pull requests

2 participants