-
Notifications
You must be signed in to change notification settings - Fork 188
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
Fix scanner force-scanning #511
Conversation
tinytuya/scanner.py
Outdated
@@ -164,6 +164,7 @@ def __init__( self, ip, deviceinfo, options, debug ): | |||
self.timeo = 0 | |||
self.resets = 0 | |||
self.step = FSCAN_NOT_STARTED | |||
self.try_v35 = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @uzlonewolf is this really only v35 or v34_and_v35? I'm trying to follow the logic/flow it seems like both, but I'm probably missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only looked at when the device version is set to 3.4, so I guess try_v35_with_v34
would be a more accurate name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! Yes, that makes sense. Thanks @uzlonewolf !
@@ -334,7 +342,7 @@ def timeout( self, forced=False ): | |||
print('ForceScannedDevice: Debug sock', self.ip, 'connect timed out!') | |||
elif self.step == FSCAN_INITIAL_CONNECT: | |||
if self.debug: | |||
print('ForceScannedDevice: Debug sock', self.ip, 'socket send failed,', 'no data received' if forced else 'receive timed out') | |||
print('ForceScannedDevice: Debug sock', self.ip, 'socket send failed,', 'no data received,' if forced else 'receive timed out,', 'current retry:', self.retries) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good addition.
Not sure if we want to address this because it is correct, but I suspect some (ok, even me) will specify a host address with netmask:
|
Hi Guys, I grabbed server.py and scanner.py (I think it was only the two files) from @uzlonewolf's commits and ran them here, they did not seem to fix my issue, ie, did not see any v 3.5 devices, have attached log and devices file files for you, hope it helps What I did notice last night I that 2 of the three devices that were not being seen by tinytuya eventually showed up on the webui, this morning, which was interesting. tinytuya scan -d >debug.log 2>&1 |
@@ -1654,6 +1654,7 @@ def add_dps_to_request(self, dp_indicies): | |||
self.dps_to_request.update({str(index): None for index in dp_indicies}) | |||
|
|||
def set_version(self, version): # pylint: disable=W0621 | |||
version = float(version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! Yes, this surfaces as an issue every 3-6mo or so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought I'd sneak that one in there :) I'm not marking it as 'closes #507' as I think there's more we can do to type-safe everything.
@MarkCupitt that force.log is still using the old scanner.py, not this modified one, as shown by:
Replace |
@MarkCupitt if you want to pull the full repo down you could do this to test the PR: # Clone
git clone https://github.com/jasonacox/tinytuya.git
cd tinytuya
# Grab PR
git pull origin pull/511/head
# Test
python3 -m tinytuya scan -f |
@jasonacox @uzlonewolf sorry, my bad, I was using the wrong lib .. so per @jasonacox
So, Im seeing 3.5 Devices now in the Force scan .. fantastic .. but some still show off line in the server web and I dont see any 3.5 Devices in the Debug log. Im not sure whether they are not sending packets, or tinytuya is missing them |
The Wireshark capture you posted to #510 show that the devices are not sending broadcasts. Since the devices are not sending packets there is nothing for TinyTuya to receive. As the device in the Wireshark capture immediately tries to find your phone after it is rebooted, I suspect Smart Life is still running in the background on your phone. |
Try to stop the SmartLife app and see if the devices start broadcasting. |
@jasonacox @uzlonewolf Yep, I actually had disabled wifi for my phone with Smartlife on it, restarted all the device. The pull request does address the 3.5 issue which was the root cause of my issue, and I accept that the devices are "different" and do something different with Broadcast packets. Ill adjust my use case to work with this approach as I will know the IP addresses and can access them directly. Im comfortable with that, so I would say the pR is Ok Thanks for the prompt fixes and responses, they are very much appreciated |
I'm going to call this issue successfully closed .. as any remaining issues appear to be device-related. Thanks again .. |
I sure hope this isn't the start of a new device series/trend that don't broadcast discovery packets. Thanks for your help with this, @MarkCupitt ! |
@jasonacox Do you want to add a "Start Force-scan" button to the server? |
Oh, that's a great idea. Hum. Let me poke at that tonight. Might be worth including in this PR. |
I also fixed the "ValueError: 10.0.1.1/24 has host bits set" thing but it looks like I forgot to push it. 1 sec |
@@ -928,7 +928,7 @@ def _generate_ip(networks, verbose, term): | |||
if tinytuya.IS_PY2 and type(netblock) == str: | |||
netblock = netblock.decode('latin1') | |||
try: | |||
network = ipaddress.ip_network(netblock) | |||
network = ipaddress.ip_network(netblock, strict=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So easy. ;-) thanks! 🙏
As a humble suggestion, If you had a force scan update
with broadcast being the Secondly, adding a timestamp recording the time it was last successfully accessed would allow for a last accessed age, thus allowing logic for retries on individual devices, should it pass a threshold, or then showing them as going offline. Because of high disk io, if running this on a pi, it may be better to keep this in memory and just serve an exact copy of snapshot.json out an API, for use in other systems. Lastly, if you were to add an optional new file, say "local_ips.json` with something like this
You could build in a poll (on a timer) like monitor.py into the scan only for non-broadcast devices shown in snapshot.json, and be able to handle most edge cases I would think Updating the UI to indicate if it is a broadcast or poll and the age [ eg: formatted DateTime (12 seconds ago) ] would also be very useful Lastly, a wishlist request, In thee UI, update the Offline devices from info in Devices.json so they are the same display as the Online Table display. This covers pretty much all the UI and oddball devices I can see from where I sit and may make Tinytuya more flexible I'm quite happy to contribute, but Python is not my first language, but it is 15 years old, however feel free to ask, dev, testing etc If you do agree to the above, I would be happy to write a separate MQTT server that uses the above (basically reads snapshot.json, from server.py) to publish values to MQTT on a regular basis, my feeling is that this is best left out of server.py, as its use case is very different, although I see there has been some thought on MQTT already, happy to contribute to that .. if you deem that a better approach The end result would be a pretty cool setup where a process server.py was running in the background keeping snapshot.json up to date, then a second process that read snapshot.json, ( looked for file changes and reload would be nice) and polled the devices (threaded or asyncio) for status and emitted MQTT publish events added #512 as an idea |
def _print_device_info( result, note, term, extra_message=None ): | ||
def _print_device_info( result, note, term, extra_message=None, verbose=True ): | ||
if not verbose: | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed while testing that verbose=False
was not honored.
scanner.devices(verbose=False, color=False, poll=False, forcescan="10.0.0.0/24",discover=False)
Printed all the devices. I updated scanner to ignore prints if verbose=False.
for bcast in client_bcast_addrs: | ||
addr = client_bcast_addrs[bcast] | ||
client_ip_broadcast_list[addr] = { 'broadcast': bcast } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be simpler to remove this logic and have send_discovery_request()
build the broadcast list itself (assume we always want it to broadcast to all interfaces)? Or do you think we will want to alter / prune the list it would use?
Either is fine... just selfishly wondering if I could just call scanner.send_discovery_request()
without worrying about sending it any arguments. 😋
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, 2 reasons: so we only need to build the interface list/encrypt the payload/open the socket once, and so we can expand the list later if needed (i.e. we're using the single getmyIP() address and want to add any address we receive a broadcast on).
It shouldn't be hard to make the send_discovery_request()
argument optional.
Ok, I think that's everything on my list for now. |
Args: | ||
verbose - Returns raw JSON data from Tuya Cloud | ||
oldlist - List of devices from previous run | ||
include_map - Include the DPS mapping in the device list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discovered I needed this better description of the args while adding the cloud sync button to the proxy. :-)
LGTM - I'm going to do a few final tests and then merge and release v1.14.1. |
@uzlonewolf We may have a problem. I just tested on a system without netifaces or psutil and I'm getting an error for the basic scan. After installing psutil, it works without any issue. We should add psutil to the setup requirements. I'll tests on a few other OS variants to make sure that works.
|
Trying: pip3 install tinytuya==1.15.0.dev0 installs psutil as a dependency. |
So it seems that pstuil doesn't work well on Windows. I switched to netifaces. pip3 install tinytuya==1.15.0.dev1 Successful tests on MacOS, Linux, RPi and Windows. |
Library: https://pypi.org/project/tinytuya/1.15.0 Server (in a can): jasonacox/tinytuya:1.15.0p13 - Run via docker, where HOST is the IP address of the host running the server (required to prevent force scan from scanning the /8 docker IP space :) docker run \
-d \
-p 8888:8888 \
-p 6666:6666/udp \
-p 6667:6667/udp \
-p 7000:7000/udp \
--network host \
-e DEBUGMODE='no' \
-e HOST='192.168.0.100' \
-v $PWD/devices.json:/app/devices.json \
-v $PWD/tinytuya.json:/app/tinytuya.json \
--name tinytuya \
--restart unless-stopped \
jasonacox/tinytuya |
Fixed in #519 . |
Nice! I'm wondering if we should reverse the dependency of netifaces. I tend to favor that to keep it clean, but having it install by default does give more functionality. Thoughts? |
I keep waffling on the netifaces dependency. On one hand it's practically a requirement for multi-interface machines, but on the other how many of our users are actually using multi-interface machines? I could go either way on it. |
The move to argparse in the last release broke the force-scan IP list.