Repository navigation
Scanner: poll configured static-IP devices that do not broadcast - #735
merodahero wants to merge 4 commits into
Conversation
jasonacox-sam
left a comment
There was a problem hiding this comment.
This is a really thorough piece of work — the test suite in particular (bounded-batch scheduling, late-broadcast invalidation, interruption cleanup, key redaction in verbose output) covers exactly the corners I'd worry about. Nice job.
A few design questions before this is ready:
1. Scan-duration cost with stale entries (main concern).
With poll=True, this now contacts every fully-configured row in devices.json on every scan, after the UDP window closes. For households where IPs rotate via DHCP, devices.json tends to accumulate stale addresses — and each stale entry costs a full TCP connect timeout inside the scan, every single time. On a 20-device list with a handful of dead entries, tinytuya scan gets visibly longer forever. Two thoughts:
- Consider an opt-out parameter (e.g.
devices(poll_configured=...)) or a way for CLI users to skip it, and/or - Only poll configured entries that look unreachable by UDP after N consecutive scans — though that adds state, so a simple opt-out may be enough.
2. Hardcoded version membership.
version not in (3.1, 3.2, 3.3, 3.4, 3.5) silently skips future protocol versions (3.6+). That's safe-by-default, which I like, but it will confuse someone whose 3.6 device is skipped with no output. A log.debug on the skip (or surfacing it in verbose mode) would save a future issue report.
3. Minor: float equality on versions.
float(item['version']) handles string versions like "3.3" fine, and tuple membership is exact for these values — no bug, just noting it's fragile if the supported-version list ever gets derived dynamically.
The UDP-authoritative reconciliation (including same-ID-at-different-IP supersede) and the StaticDevice.close() "only a decoded status response counts as found" rule are both the right calls. — Sam ⚙️
|
Thanks for the close read, Sam — those are great questions. 1. Stale-entry cost: agreed. I’ll take the simple stateless path and add a compatibility-preserving opt-out: 2. Future versions: agreed. I’ll emit a debug message when an otherwise complete configured row is skipped for an unsupported protocol version, without including its local key. 3. Float equality: noted. TinyTuya’s device APIs currently use float protocol versions, so I’ll keep that representation but centralise the pinned supported-version tuple and document that exact membership is intentional for the literal 3.1–3.5 set. I’ll add focused regression coverage and update the public API/CLI documentation in the same follow-up commit. |
|
That plan covers all three points cleanly — the stateless opt-out is the right call (no hidden scan-history state, no surprise — Sam ⚙️ |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved scanner validation, lifecycle, termination, cleanup, and snapshot-handling issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds bounded static-IP polling for configured Tuya devices that do not broadcast, with UDP precedence and API/CLI controls.
Changes:
- Adds validated static polling and result reconciliation.
- Updates APIs, CLI, tests, and documentation.
- Adds release notes and scanner regression coverage.
File summaries
| File | Summary |
|---|---|
tinytuya/scanner.py |
Static polling orchestration and reconciliation |
tinytuya/core/core.py |
Public API forwarding |
tinytuya/__main__.py |
CLI opt-out flag |
tests.py |
Static scanner regression coverage |
RELEASE.md |
Release note |
README.md |
User documentation |
API.md |
API documentation |
Review details
Suppressed comments (6)
tinytuya/scanner.py:1075
abort()closes the socket but leavesPollDevice.retriesat 2. If a UDP socket and this static socket are both ready in oneselect()result, the UDP branch aborts it and the later read branch callsread_data()withself.sockset toNone; its exception path then retries and reconnects, undoing the abort. Make an aborted static poll non-retryable or skip removed sockets before dispatch.
def abort(self):
"""Prefer a late UDP broadcast over the configured fallback."""
self.found = False
DeviceDetect.close(self)
tinytuya/scanner.py:1366
- This guard still accepts a truthy non-string key, so a row such as
{'id': '...', 'key': 123}reachesKeyObj, whose constructor calls.encode()and aborts the entire scan before malformed-row handling can be non-fatal. Apply the same string checks used for static candidates when buildingkeylist.
if isinstance(i, dict) and i.get('id') and i.get('key'):
options['keylist'].append(KeyObj(i['id'], i['key']))
tinytuya/scanner.py:1795
maxdevicesis decremented only in the UDP-discovery branch, but this loop starts configured candidates without considering the remaining limit. With no broadcasts,devices(maxdevices=1, ...)can therefore successfully static-poll and return every configured device instead of stopping after one; carry the limit through static result acceptance as well.
started = 0
while (configured_poll_index < len(configured_candidates) and
started < want):
deviceinfo = configured_candidates[configured_poll_index]
tinytuya/scanner.py:1811
- This
trycatches ordinary exceptions, but static scheduling runs outside the earlierKeyboardInterrupthandler and the function has no surroundingfinally. A Ctrl-C duringStaticDeviceconstruction orconnect()can therefore exit without reaching the reconciliation/close block, leaving the newly opened socket (and existing scanner sockets) uncleaned.
dev = StaticDevice(ip, deviceinfo, options, ip in debug_ips)
configured_devices[ip] = dev
known_ids.add(deviceid)
devicelist.append(dev)
started += 1
try:
dev.connect()
except Exception:
log.debug("Unable to start configured poll for %s", ip)
tinytuya/scanner.py:1903
- Static successes are counted here but never removed from
wantipsorwantids, unlike UDP results above. A requested device found only through its configured IP is returned successfully yet the verbose summary still reports it as missing; reconcile those filter lists when accepting a static result.
configured_found_count = 0
for ip in configured_devices:
dev = configured_devices[ip]
if not dev.found:
continue
configured_found_count += 1
tinytuya/scanner.py:1190
- Making
devices()default topoll_configured=Truealso affects the existingsnapshot()andsnapshotjson()callers, which passsnapshot=by_ipwithout opting out and therefore loaddevices.jsonand contact every eligible configured IP. That changes the documented snapshot-only commands to reach stale/unlisted addresses, while their rendering ignores newly found rows. Keep the fallback disabled for those snapshot callers or make the scope explicit.
def devices(verbose=False, scantime=None, color=True, poll=True, forcescan=False, byID=False, show_timer=None,
discover=True, wantips=None, wantids=None, snapshot=None, assume_yes=False, tuyadevices=None,
maxdevices=0, poll_configured=True): # pylint: disable=W0621
- Files reviewed: 7/7 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if not all(item.get(field) for field in ('id', 'key', 'ip', 'version')): | ||
| return None |
| configured_candidates = [ | ||
| deviceinfo for deviceinfo in | ||
| (_configured_device_info(item) for item in tuyadevices) | ||
| if deviceinfo | ||
| ] |
| if (configured_poll_pending and not user_break_count and | ||
| not ip_scan_running and scan_end_time <= time.time()): |
| if (configured_poll_pending and not user_break_count and | ||
| not ip_scan_running and scan_end_time <= time.time()): |
|
Went through Copilot's latest pass against the branch — I agree with most of it, with one scoping note. In rough priority order: Agreed, worth fixing in this PR:
Scoping note — pre-existing, not introduced here: the truthy-only checks in the — Sam ⚙️ |
Validate malformed device rows across scanner consumers, respect early-stop and device budgets, drain active force scans, and make static cancellation and interrupt cleanup reliable. Keep snapshot commands scoped to their saved addresses. Add regression coverage and update API, README, and release documentation. Verified with 37 focused tests, 75 full-suite tests (one skipped), Contrib, pylint, and isolated-source protocol-3.5 hardware polling.
|
Addressed the scanner review findings in b46f00f, with regression tests and documentation updates:
The default-enabled fallback and API/CLI opt-out remain unchanged. Local verification (Python 3.12): 37 focused scanner tests passed; the full suite ran 75 tests with one skipped and no failures. Contrib checks, compileall, errors-only pylint, and Hardware verification: ran the exact patched source from an isolated temporary directory against a non-broadcasting protocol-3.5 meter. With the fallback disabled, discovery returned five broadcast devices and omitted the meter. With it enabled, discovery returned those five plus the meter as The new upstream Actions runs currently report |
|
This is exactly the kind of follow-through a scanner change like this needs — thank you. Validating against a real non-broadcasting v3.5 meter is the case the original PR description cared about most, so having that hardware-confirmed is great to see. The regression suite (37 scanner tests, full run green) plus the lifecycle hardening in b46f00f/840289d/b3c93ba addresses everything flagged in the review round. One note for the maintainers: the upstream CI run currently shows — Sam ⚙️ |
Summary
DeviceDetect/PollDevice/select()state machine, bounded bymax_paralleland ten configured starts per event-loop passProblem
Some Tuya devices accept authenticated TCP/6668 connections but do not send a usable UDP discovery response. TinyTuya can therefore have every value needed to poll the device in
devices.jsonand still omit it from an ordinary scan.This behavior was observed with a protocol 3.5 breaker/meter. A direct
OutletDevice.status()call succeeds, while UDP and forced discovery do not identify it. No device-specific values are included in this change.This follows the non-broadcasting-device discussion in #511, particularly the concern that silent devices might become a broader firmware trend and the suggestion to poll known IPs directly.
Approach
UDP discovery and explicit force scanning run first. Afterwards, device-list rows are eligible only when they contain a non-empty ID and local key, a valid IPv4 address, and a supported protocol version (3.1-3.5).
Eligible devices enter the existing non-blocking scanner lifecycle in bounded batches. A late UDP announcement supersedes a pending, in-flight, or completed static result by device ID or IP. Matching static sockets/results are closed and removed before result shaping. Failed static polls are omitted without aborting the scan, and remaining sockets are cleaned up on normal finalisation or interruption.
Successful fallback results carry
origin = "static"and participate once in verbose totals, protocol-version counts, return data, and snapshot output. Verbose static output uses a redacted copy of device metadata.Tests
Verified locally:
python3 -m unittest tests.TestConfiguredDeviceScanner -v- 15 passedpython3 -m tests- 53 run, 52 passed, 1 skippedpython3 -m compileall -q tinytuya- passedpython3 testcontrib.py- passedpylint --recursive y -E tinytuya/- passedgit diff --check- passedRegression coverage includes candidate validation, alternate device files, bounded later-round scheduling, failed connects/polls, same-ID late UDP at a different IP, completed-result invalidation, interruption cleanup, verbose reconciliation/key secrecy, and a real
StaticDeviceconnect/write/read lifecycle with only socket/wire primitives mocked.Hardware validation
Validated the final branch source against a real protocol 3.5 device that:
[Static Poll]withorigin = "static";Private device identifiers, addresses, MACs, keys, names, and DPS values are intentionally omitted.
Risk
The change is limited to scanner discovery/poll orchestration, adds no dependency, and reuses TinyTuya's authenticated read-only polling path. Only explicitly configured, strictly validated devices are contacted, after the ordinary discovery window, with bounded concurrency.