-
Notifications
You must be signed in to change notification settings - Fork 172
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
Complete rewrite of the scanner, now allows force-scanning of IP ranges #252
Conversation
# Enable UDP listening broadcasting mode on encrypted UDP port 6667 - 3.3 Devices | ||
clients = socket.socket(socket.AF_INET, socket.SOCK_DGRAM, socket.IPPROTO_UDP) | ||
clients.setsockopt(socket.SOL_SOCKET, socket.SO_BROADCAST, 1) | ||
clients.bind(("", UDPPORTS)) |
Check warning
Code scanning / CodeQL
Binding a socket to all network interfaces
#clients.settimeout(TIMEOUT) | ||
clientapp = socket.socket(socket.AF_INET, socket.SOCK_DGRAM, socket.IPPROTO_UDP) | ||
clientapp.setsockopt(socket.SOL_SOCKET, socket.SO_BROADCAST, 1) | ||
clientapp.bind(("", UDPPORTAPP)) |
Check warning
Code scanning / CodeQL
Binding a socket to all network interfaces
Thank you @uzlonewolf !! Merging this and versioning it v1.9.2 for now. Speed improvement is incredible, LW! Blow away. So, I was able to produce a few error conditions we shouldtreat. Python3This happened on first run - unable to reproduce.
Python2This happened on first run, ran without flaw next several times, then after several tries later, again.
Also on Python2 I left netifaces uninstalled so I see this:
I now want to add a I'll keep running test. |
I recently discovered a module that adds tab-completion to argparse, so I'll most likely rewrite __main__.py to use that soon :) I'll see if I can fix those errors tomorrow. |
Love that! I have some older 3.1 devices that stop broadcasting after a while but are otherwise working. They are discovered with
Thanks LW. Some of the errors seem to coincide with MacOS prompting for permission for python to listen to the network. I'm guessing a timeout condition we could catch. I haven't dug in yet. No rush. This is beautiful! |
All those errors are fixed in #254 . At some point I'm going to rewrite the wizard to use the new scanner. If you pass a list of device IDs into |
Thanks @uzlonewolf ! I can't reproduce those errors now so they do appear fixed. However, I ran into some others with the rest of the testing: The snapshot.json seems to have duplicate records after the scan (but missing IP):
On python3, it actually throws some exceptions after rendering the table - this was repeated several times and likely related to the first two device entries w/o keys (these are new devices I have added and not pulled down keys with wizard yet just to provide a test). Oddly, python2 doesn't throw the errors but the table has the duplicate devices (with 2nd one missing IP).
Since Now, if you run
Yes! |
BTW, what I like doing when adding a new device is starting a really long scan such as 1st <ctrl>-c - Timer is cancelled and force-scan is stopped. Any in-flight status requests are allowed to continue. |
Nice! Your latest PR: 3.1 device discovery is fixed! The remaining bug can be reproduced by doing this:
This happens for each device:
The issue is that there is no local key. There are many places where we could check for a missing key and inject one or branch an error. I believe the fix is likely in |
Oops... we have a global name typo. I ran a test against a 3.1 device: import tinytuya
d = tinytuya.OutletDevice(DEVICEID, DEVICEIP, DEVICEKEY)
print(d)
d.set_version(3.1)
print(d.status()) Traceback (most recent call last): That PREFIX_BIN = PREFIX_55AA_BIN = b"\x00\x00U\xaa" |
Ok, I'm not super happy with the fix (using a I also set the display logic in |
Last odd bit I'm tackling -
Force seems to zero out the gwId in certain cases. I don't know if this is a bug or the fact that the force scan found the device before the broadcast with the ID comes through.
|
Even if force-scan finds it first, if it also gets a broadcast then it uses said broadcast since those packets have more information in them. In my testing, the null-id issue only happens when all 3 conditions are met:
I'm working on making the force-scan a bit more robust, but there's nothing that can be done if the device is unknown and does not have a known local key. As a kludge we can delay the force-scan by ~6 seconds or so to give it a chance to broadcast, or increase the scan time to force-scan time + max time until forced-stop + ~6. |
That makes sense. I have about 27 Tuya devices in my test. I can get it to work if I force scantime to 45s but doesn't find them all if I lower it to 30s (misses 15). At 60s, all of the broadcast come in except for 2 older 3.1 devices that have a tendency to go into a bad state and stop broadcasting anyway. That is getting very close to 100% blind discovery using -force. Key point you made is that this edge case is only when we are missing vital devices.json data. My suggestion: I don't want to arbitrarily increase the scantime, but if the user is missing the devices.json file and doesn't specify a scantime, we could auto-increment it with a warning, "NOTICE: No devices.json file found (run wizard to generate), increasing scantime to 60s to increase discovery." It won't help with VLAN boundaries blocking broadcasts, but should capture the majority of devices for most users. I'm probably way over thinking this but my hunch is that many users who install tinytuya fire up a scan even before running wizard. |
IMO using force-scan without any devices.json at all is an illegal combination and as such we should throw an error. Due to devices not responding unless you know the key it is useless unless you just want a (very poor) port scanner. If not an error then we should at least print a warning telling the user that force-scan won't work without devices.json. I'm thinking something like: if not len(devices) and forcescan:
if not discover:
raise 'Force-scan requires a devices.json'
else:
forcescan = False
print 'Warning: Force-scan requires devices in devices.json. Force-scan disabled, only using passive broadcast discovery' |
Well said. 😁 Excellent points. Sold! Does this approach raise an exception for non-CLI initiated scans w/o devices.json but automatically disables force-scan for CLI initiated scans? If so, I love it. I would like to provide friendly (not exception trace output) for command line users where possible. But in any case, agree on approach to avoid the "illegal combination" (which translates to less support issues to answer 😂 ). |
Not really. It doesn't care about CLI-vs-non-CLI. If it was called as the equivalent of |
I get it. The point being that you can't downgrade to broadcast if "-no-broadcasts" requested. |
So, how close do we need the snapshot file to be between force-scanned devices and broadcasted devices? Currently broadcasted devices have "version" set to "3.3" (string) and no "ver" while force-scanned devices have "version set to 3.3 (float) with "ver" also set to 3.3. |
Ugh, we should try to converge. There shouldn't be a need to have both. I suggest we:
The [
{
"ip": "10.2.3.44",
"origin": "broadcast",
"gwId": "0123456789abcdef0123",
"active": 2,
"ability": 0,
"mode": 0,
"encrypt": true,
"productKey": "MShdslm9Uw7Q59nN",
"version": "3.3",
"name": "Light Switch",
"key": "0123456789abcdef",
"mac": "c1:d2:e3:a9:f5:06",
"ablilty": "",
"token": "",
"wf_cfg": "",
"dev_type": "default",
"err": "",
"type": "default",
"dps": {
"devId": "0123456789abcdef0123",
"dps": {
"1": true,
"9": 0
}
}
}
] Thoughts? |
I do prefer "version", however the older snapshot.json used "ver" (and "id" instead of gwId"). At least in some places. I remember it being a real mess, which is why I had it saving both. From a snapshot file created by 1.9.1:
I already do some normalization when reading from it, it wouldn't be difficult to do the same thing when saving. And I really like that |
Oh yes, you are right... just checked the old snapshot example code I created a few years ago. Embarrassing. 😊 item["name"],
item["id"],
item["ip"],
item["key"],
item["ver"] I see your PR and agree. Let's stick with this for the snapshot and do the conversion with your functions to help normalize the data. It will keep from breaking any code that others have built based on the older format and example. |
Hey @uzlonewolf, found a minor / odd edge case on a Windows 11 system with the scanner (snapshot). I'm using a devices.json file that is intentionally missing a few new devices and has the wrong key for at least one (my typical test pattern). I'm getting this error repeatedly for snapshot (no errors in scan), but it then displays the expected table including the test case, "No response" and "No IP found" devices.
I can't reproduce this on MacOS or Linux. We could add a try/except block to eliminate the noise, but it is odd that it shows up only on one platform. |
It's in a try/except block, it's just that there is a print(traceback) in the except. Perhaps wrap that print() in a |
Full discussion is in #177. Created a new PR to keep the changelog clean.
Closes #159
Closes #172
Closes #232