Skip to content

Scanner: poll configured static-IP devices that do not broadcast - #735

Open
merodahero wants to merge 4 commits into
jasonacox:masterfrom
merodahero:fix/scanner-static-device-poll
Open

merodahero wants to merge 4 commits into
jasonacox:masterfrom
merodahero:fix/scanner-static-device-poll

Conversation

@merodahero

@merodahero merodahero commented Sep 3, 2026 •

Copy link
Copy Markdown

Summary

  • poll fully configured static-IPv4 devices when UDP discovery does not identify them
  • reuse TinyTuya's existing DeviceDetect / PollDevice / select() state machine, bounded by max_parallel and ten configured starts per event-loop pass
  • keep UDP authoritative, including late announcements for the same device ID at a different IP
  • validate candidates before socket construction and keep malformed rows or failed polls non-fatal
  • reconcile static results, verbose totals, version counts, and snapshots without duplicates or local-key output

Problem

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.json and 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 passed
  • python3 -m tests - 53 run, 52 passed, 1 skipped
  • python3 -m compileall -q tinytuya - passed
  • python3 testcontrib.py - passed
  • pylint --recursive y -E tinytuya/ - passed
  • git diff --check - passed
  • Python 3.7 container full suite - 53 run, 52 passed, 1 skipped
  • Python 3.12 container full suite - 53 run, 52 passed, 1 skipped
  • sabotage check disabling configured-candidate selection - central regression failed as expected, then passed after restoration

Regression 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 StaticDevice connect/write/read lifecycle with only socket/wire primitives mocked.

Hardware validation

Validated the final branch source against a real protocol 3.5 device that:

  • has TCP/6668 open;
  • returns live DPS through the configured static poll;
  • does not provide a usable UDP discovery response;
  • appears once as [Static Poll] with origin = "static";
  • is counted once in device and protocol-version totals;
  • does not expose its local key in captured output.

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.

@jasonacox-sam jasonacox-sam left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ⚙️

@merodahero

merodahero commented Sep 9, 2026 •

Copy link
Copy Markdown
Author

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: poll_configured=True as a final API parameter, plus scan -no-poll-configured for CLI users. The default will retain the fallback introduced here; opting out will leave ordinary UDP/forced discovery intact while skipping configured static-IP polls. I won’t add scan-history state or mutate devices.json.

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.

@jasonacox-sam

Copy link
Copy Markdown
Collaborator

That plan covers all three points cleanly — the stateless opt-out is the right call (no hidden scan-history state, no surprise devices.json mutation), and skipping the key in the debug message is exactly the right instinct. Looking forward to the follow-up commit.

— Sam ⚙️

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 leaves PollDevice.retries at 2. If a UDP socket and this static socket are both ready in one select() result, the UDP branch aborts it and the later read branch calls read_data() with self.sock set to None; 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} reaches KeyObj, 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 building keylist.
        if isinstance(i, dict) and i.get('id') and i.get('key'):
            options['keylist'].append(KeyObj(i['id'], i['key']))

tinytuya/scanner.py:1795

  • maxdevices is 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 try catches ordinary exceptions, but static scheduling runs outside the earlier KeyboardInterrupt handler and the function has no surrounding finally. A Ctrl-C during StaticDevice construction or connect() 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 wantips or wantids, 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 to poll_configured=True also affects the existing snapshot() and snapshotjson() callers, which pass snapshot=by_ip without opting out and therefore load devices.json and 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.

Comment thread tinytuya/scanner.py Outdated
Comment on lines +1085 to +1086
if not all(item.get(field) for field in ('id', 'key', 'ip', 'version')):
return None
Comment thread tinytuya/scanner.py
Comment on lines +1356 to +1360
configured_candidates = [
deviceinfo for deviceinfo in
(_configured_device_info(item) for item in tuyadevices)
if deviceinfo
]
Comment thread tinytuya/scanner.py Outdated
Comment on lines +1761 to +1762
if (configured_poll_pending and not user_break_count and
not ip_scan_running and scan_end_time <= time.time()):
Comment thread tinytuya/scanner.py Outdated
Comment on lines +1761 to +1762
if (configured_poll_pending and not user_break_count and
not ip_scan_running and scan_end_time <= time.time()):
@jasonacox-sam

Copy link
Copy Markdown
Collaborator

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:

  1. Early-termination gap — ip_force_wants_end isn't checked in the static-poll launch condition. Once wanted IPs/IDs are found (or maxdevices is hit), scan_end_time is zeroed, which makes scan_end_time <= time.time() immediately true — so stale configured addresses can still be contacted after the caller asked the scan to stop. Adding not ip_force_wants_end to the gate looks like the clean fix.

  2. maxdevices not enforced on the static path — the limit is only decremented in the UDP-discovery branch, so devices(maxdevices=1, poll_configured=True) with no broadcasts can return every configured device. Carrying the remaining count through static result acceptance seems right.

  3. Force-scan drain race — when the force-scan generator is exhausted, ip_scan_running goes false while in-flight ForceScannedDevice objects are still in devicelist, and the duplicate check only inspects the broadcast/scanned/configured maps — so a StaticDevice can open on the same IP and be counted separately. Including active devices' IPs/IDs in the eligibility check (or draining first) resolves it.

  4. abort() retry race — if a UDP socket and a static socket are both ready in one select() pass and the UDP branch aborts the static poll, the later read branch calls read_data() with self.sock already None, and its exception path can retry/reconnect, undoing the abort. Skipping sockets that were closed/removed during dispatch (or making an aborted poll non-retryable) both work.

Scoping note — pre-existing, not introduced here: the truthy-only checks in the keylist loop and the verbose snapshot loop's item["id"] indexing are identical to upstream master (they predate this PR). Hardening them is worthwhile, but I'd treat that as a separate follow-up rather than expanding this PR's diff — malformed devices.json rows already crash verbose scans on master today.

— 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.
@merodahero

Copy link
Copy Markdown
Author

Addressed the scanner review findings in b46f00f, with regression tests and documentation updates:

  • Validate ID/key types before static candidate and key-list construction; skip malformed rows in snapshot assembly and scanner lookups.
  • Make static cancellation non-retryable and ignore stale select() events for closed/replaced sockets.
  • Stop configured scheduling after early termination, reserve maxdevices capacity for in-flight polls, and release slots after failed polls. Broadcast and force-scan results consume the same fallback budget.
  • Wait for active force scans to drain and register newly completed results before static duplicate checks.
  • Reconcile requested IDs/IPs during polling and final reporting. Explicit force scans still run first; configured polling precedes the implicit force scan of missing requested IPs.
  • Add finally cleanup for tracked scanner sockets, including connections opened since the previous select() pass. Cover interrupts during construction, connection, and the first select() call.
  • Keep snapshot() and snapshotjson() scoped to saved snapshot addresses by explicitly disabling the configured fallback.

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 git diff --check passed. Python 3.7 syntax compatibility was also checked (not a 3.7 runtime test).

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 origin=static with 17 DPS fields. Direct status succeeded before and after. The installed package, device/snapshot files, and breaker state were unchanged; temporary source cleanup was verified.

The new upstream Actions runs currently report action_required; local and hardware results above are separate from upstream CI.

@merodahero
merodahero marked this pull request as ready for review September 14, 2026 10:23
@jasonacox-sam

Copy link
Copy Markdown
Collaborator

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 action_required — likely pending workflow approval, so it may need a re-run or approval before it goes green.

— Sam ⚙️

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

Successfully merging this pull request may close these issues.

3 participants