Binary-protocol build-out, video/reconnect robustness, and test tooling (0.3.0) - #13
Open
DevLn wants to merge 107 commits into
Open
Binary-protocol build-out, video/reconnect robustness, and test tooling (0.3.0)#13DevLn wants to merge 107 commits into
DevLn wants to merge 107 commits into
Conversation
control() returns None (it already awaits the ACK internally), so the subsequent `await self.wait_ack(idx)` passed idx=None and tripped the "Need to provide numeric command index" guard, surfacing an error to the caller even though the IR command was sent. Drop the redundant wait. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
stop() raised RuntimeError whenever state != CONNECTED, but it is called from several paths (_on_device_lost, Device.close, the CLI shutdown loop) and can run twice or on a session that never finished connecting. A second call then aborted the surrounding cleanup. Return early when already disconnected and guard each task/transport before touching it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`return response` inside a `finally` suppressed every in-flight exception, including the asyncio.CancelledError raised when the client disconnects. That defeated task cancellation and kept the streaming coroutine alive. Let the loop exit normally and return afterwards so cancellation propagates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The command index is packed into a 16-bit wire field and DRW ACKs only carry 16 bits. Using an unbounded counter meant that after 65536 commands sends raised struct.error and, before that, ACK matching could never line up. Mask the counter to 0xFFFF so it wraps like the protocol expects. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_build_video_param resolves the enum class via globals()['VideoRotate'], but VideoRotate was never imported into session.py, so the web-UI rotate control raised KeyError. Add it to the const import. Note: JsonSession still does not implement set_video_param; the JSON video-parameter controls remain unsupported (separate follow-up). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The discovery-task cleanup belongs in `finally`, but the `return`/`raise` that decides the result was inside it too, so a CancelledError from the outer await was suppressed (Python even emitted a SyntaxWarning). Keep only the cleanup in `finally` and resolve the result afterwards. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
on_device_found/on_device_lost called set_result() unconditionally on the shared new_device_fut. It is None before the main loop creates it and is recreated each iteration, so a second discovery callback (or one firing before the loop started) raised InvalidStateError/AttributeError. Funnel both through a guarded helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Every incoming UDP packet on XOR1 cameras was decoded with a per-byte Python loop, the dominant CPU cost while streaming. The keystream byte at position i depends only on the previous ciphertext byte, and for decode all ciphertext is known up front, so the keystream can be produced with a single bytes.translate() over a precomputed 256-entry table and applied with one big-integer XOR. Output is bit-for-bit identical to the previous implementation (verified across random and edge-case inputs); decode of a 1400-byte packet is ~28x faster. Encode stays a tight loop (sequential, and only used for small outgoing packets) but reuses the same precomputed table. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
process_video_frame ran on every received chunk and rebuilt the payload list plus a diagnostic completeness string each time, even while the frame was incomplete. It also only pruned old chunks when a frame *completed*, so a frame left permanently incomplete by packet loss made video_received and video_boundaries grow without bound (measured: 4509 retained chunks vs 18 after this change on a lossy stream). Now the completeness check stops at the first gap and the payload/debug string are built only when needed, and chunks/boundaries below the current frame start are dropped on every call. Only the last two boundaries are ever assembled, so this cannot change which frames get published — verified bit-for-bit against the previous behavior across 200 randomized lossy streams. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Discovery picked a random local port in 0x800-0xfff0 and bound it with no retry. On Windows that port can fall inside an OS-reserved exclusion range, so the bind fails with PermissionError (WinError 10013) and takes the whole discovery/server down. Bind to port 0 instead (honoring an explicit local_port when set) so the OS hands back a guaranteed-free port, and log the chosen port. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
amain() created Discovery(remote_addr=...) without passing local_port, so the -dp/--local-discovery-port CLI flag was parsed but never applied — the discovery socket always used the default. Pass local_port through so the flag actually pins the port. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Expose an optional callback that fires whenever a session's video stream starts or stops, so consumers (e.g. the Home Assistant integration) can reflect the real streaming state instead of assuming it is always on. The callback receives the new is_video_requested value and fires on start_video, stop_video, and on session teardown (so "streaming" clears when the session ends for any reason, including a stalled-stream drop). Threaded through Session, make_session, and Device; defaults to None and is fully backward-compatible. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Binary cameras had no dead-peer detection at all: BinarySession.loop_step only sent P2PAlive, the video-stale disconnect logic is JSON-only, and the P2PAliveAck was received but ignored. A camera that silently dropped left a zombie session that kept sending keepalives forever, was never reported as lost, and never reconnected. Track the time of the last datagram received from the camera (any packet -- video, P2PAlive, ACK -- counts as proof of life) and, in the base loop_step, tear the session down if nothing arrives for RECV_TIMEOUT_SEC (20s). This works for both protocols. Also return from JsonSession.loop_step after a video-stale disconnect so it no longer falls through to the base step on an already-closed transport. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Discovery re-finds known cameras on every cycle and on_device_found created a fresh session each time, overwriting SESSIONS[dev_id] without stopping the previous session -- leaking its running tasks (each kept sending keepalives). Skip when a session for that dev_id already exists; a lost device is removed from SESSIONS first, so reconnect on the next discovery still works. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
If a camera answered discovery but never completed the P2pRdy handshake (a flaky/half-wedged camera), the wait_for() in _run() raised asyncio.TimeoutError that was not caught (the surrounding try only handled CancelledError). The session task died with that exception, and the test server's amain() re-raised it via gather() and exited -- the whole web server crashed. Catch the P2pRdy timeout (treat it as a lost device, like the setup-device timeout already is) and add a catch-all so any unexpected session error tears that one session down instead of taking the process with it. Also fix stop() to clean up a session that started connecting but never reached CONNECTED (state still DISCONNECTED but with a live transport + queue tasks), which the previous idempotency guard skipped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
These cameras ignore the resolution set at stream-start and adaptively downgrade a few seconds in. Re-selecting the resolution mid-stream from the UI is known to make it stick, so mimic that: after CMD_PEER_LIVEVIDEO_START, schedule a delayed (5s) re-send of CMD_PEER_VIDEOPARAM_SET. Guarded so it no-ops if the stream was stopped during the wait. Experimental -- the lock behaviour and delay are based on observed camera behaviour and may need tuning per device. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Track the DRW index/epoch from Video-channel packets only. The camera counts the index independently per channel, so mixing command/audio indices into the wraparound check could spuriously flip video_epoch and shift chunk indices by 0x10000, corrupting frame reassembly (the likely root cause of "binary video not working"). - Lift the duplicated handle_drw/_get_drw_epoch into the base Session and dispatch command/audio channels through overridable hooks. - Bound drw_waiters and cancel wrap-collided/evicted futures so fire-and-forget commands (reboot, toggle_*, PTZ) can't leak ACK waiters. - Replace cmd_waiters on re-request via _reset_cmd_waiter so an unanswered response future can't orphan its awaiter; stop creating unused response waiters on the video start/param path. - Track the video-param re-assert task so it can't be GC'd mid-flight and is cancelled on stop(). - Fix _build_video_param for enum param types and enum/string values. - Rotate xq_bytes_encode/decode modulo the payload length so 1-3 byte payloads round-trip. - Never let a malformed datagram raise out of the UDP receive callbacks: guard parse_packet's type lookup and drop undecodable packets in both the session and discovery receive paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
BinaryCommands mixed config-section IDs (CFGID_*), ACK status codes (CMD_ACK_*) and the CGI vocabulary (CB_*) in with the real command opcodes, producing many duplicate values (e.g. CFGID_VERSION and CMD_ACK_OK both 0x0000; CB_IEGET_* aliasing CMD_NET_* at 0x6001-0x6005). Python silently turns the second name into an alias, so BinaryCommands(n) could never resolve to some names and parse_drw_pkt mislabelled aliased commands. Extract three separate enums -- DevCfgId, AckCode and CgiCommands -- so every enum now has strictly distinct values and value->name lookups are unambiguous. BinaryCommands keeps only real opcodes (196 distinct). No callers referenced the moved names (only const.py), so this is internally contained. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
process_video_frame ran on every incoming chunk and, for the pending frame, both rescanned the whole [index, last_index) range for completeness and scanned all received chunks/boundaries to prune. For a large keyframe spanning many chunks that is O(frame^2) CPU per frame. Track the assembling frame's window and its set of still-missing chunk indices. A chunk that lands in the current window just discards its index from the missing set (O(1)); the O(frame) missing-set recompute and the prune now run only when the window advances (once per frame). Frame output is byte-identical. Also hoist the frame marker to a module constant and reset the new tracking state in stop_video(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Binary sessions only had the base receive-timeout liveness check, which a camera that keeps ACKing P2PAlive while sending no video passes forever -- the stream silently zombies. Move the JSON video-stall logic (re-request after 5s of no DRW frames, disconnect after a further 10s) into the base Session.loop_step so both protocols share it, and drop the now-redundant JsonSession/BinarySession loop_step overrides. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The binary session blind-toggled IR and white light (ignoring the requested value) and stubbed reset/lamp. Send an explicit on/off state in the *_ONOFF payload, implement reset via the default-config recovery command, alias lamp to the fill light, and add get_snapshot via CMD_SNAPSHOT_GET. Extend ACKS/REV_ACKS so IRCUT/LIGHTFILL/REBOOT/SNAPSHOT results correlate instead of being uncorrelated fire-and-forget. Payload/response framing for these commands is derived from the decompiled app dispatch and unverified against hardware. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add ptz_goto_preset/ptz_set_preset on top of the existing direction-based PTZ, reusing the passthrough PTZ frame with the preset index in the third field and PRE_TO/PRE_REC as the direction. Preset encoding derived from the decompiled app PTZ constants and unverified against hardware. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
get_possible_discovery_packets() only emitted the plain LanSearch (0x30). Some cameras/firmwares answer only the extended LanSearchExt (0x32) -- the type was already defined in PacketType but never sent -- so those devices were silently undiscoverable. The vendor apps broadcast both; now we do too, each type once per known transport encryption (4 packets total). Verified no regression: the four known PTZA/FTYC cameras still discover (enc=NONE, binary). LanSearchExt is empty-bodied like LanSearch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Everything from the real-device test campaign: FTYC video (audio-mux demux, epoch fix), two-way audio, datetime layouts, status decoding, LanSearchExt discovery, web-UI overhaul. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
parse_dev_status rendered the status block's timezone field unconditionally, so FTYC -- which leaves a constant 224 there -- displayed 'UTC-1' (224 floor-divided by 3600). parse_datetime_block already guarded against this; both now share a _tz_west_seconds() helper so they cannot disagree again, and 'tz' is None (plus a new utcOffsetSeconds) when the value can't be a timezone. Also adds BinarySession.decode_video_param()/get_video_param_value(), which centralise the table/pair/scalar VIDEOPARAM_GET shapes that the web UI was decoding inline, so other consumers (Home Assistant) can read a parameter without duplicating that logic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A fully-charged FTYC sitting on the charger reported 60% indefinitely. The cause was the mapping, not the reading: batPercent reused the vendor app's batImgGet thresholds, but those select one of five battery ICONS (>=4350 / >=4200 / >=4100 / >=3950 / >=3900) -- the third icon is not '60 percent'. Anything under 4200 mV was pinned at 60% or lower, so a cell at its 4.2 V charge ceiling could never read full. batPercent now interpolates a single-cell LiPo resting-voltage curve, which is monotonic across the whole 3000-4600 mV range. Verified against the device: 4197 mV now reads 100% (was 60%). Mains-only cameras still return None (they park the field at 8000). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Upstream's last published release is 0.2.3 (PyPI, Oct 2025), so 0.3.0 is the next minor. Device testing had bumped this twice -- 0.3.0 and then 0.4.0 -- but neither number was ever published, and shipping 0.4.0 would imply a 0.3.0 release that does not exist. Collapse both bumps into one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three things had drifted from the code: - batPercent was described as using "the app's own thresholds". It no longer does -- those thresholds pick one of five battery icons, and reading them as percentages pinned a charged cell at 60%. It is now a LiPo discharge curve, and returns None for the 8000 placeholder that mains-only cameras park in the field. - Discovery sends LanSearchExt as well as LanSearch; some firmwares only answer the extended probe. - Added a Time sync column to the device table. FTYC is partial: it sets the clock but has no timezone field to write. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
start_video() sends a hardcoded HD parameter and re-asserts it ~5s in. The re-assert is deliberate -- the cameras self-downgrade and ignore the value set at stream start -- but it also discards a resolution chosen beforehand, and the stall-recovery re-request can revert a running stream to HD. Documented rather than fixed, per request. The note records the shape of the fix (a per-session preferred resolution) so it isn't re-derived. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Upstream set both 'uptime' and 'dbm' from this field, with the comment "not sure if that is wifi dbm or system uptime". A previous commit here resolved that the wrong way and dropped 'dbm'. Both vendor apps settle it. Every read of getSysUptime() in FtyCamPro and YsxLite goes straight to setWifidbm(), and it is rendered by wlanSigGet(), whose buckets are RSSI ranges (-100/-85/-70/-55). Neither app displays an uptime anywhere. The SDK bean's field name is simply a misnomer. So publish 'dbm' (clamped to a plausible -127..-1, else None) and drop 'uptime'/'uptimeText' along with the now-unused _fmt_uptime helper. Details, with file/line citations: VENDOR_APP_FINDINGS.md Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Closing for now -- re-checking the vendor apps turned up a decoding bug in this branch: the device-status field the ilnk SDK names The same review surfaced a couple of other things worth folding in ( Will reopen once it's fixed and re-verified. Sorry for the noise. |
swVer and powerSupply are packed words, not the plain version and mains
bit we treated them as. Unpack them and put names on the pieces, using
the model/capability enums both vendor apps ship:
swVer byte 1 devType, byte 3 chipType
powerSupply bit 0 external power, bits 4-7 sysMode,
bits 24-31 a live function bitmap
funcBmp bits 0 and 1 are confirmed on FTYC hardware: toggling IR moved
the byte 0x14 -> 0x16 and the light button 0x16 -> 0x17, with nothing
else in the 124-byte block changing. So 'lamp' now reports the real
white-light state instead of a hardcoded 0.
Not every firmware fills the word in. PTZA leaves powerSupply entirely
zero, which would otherwise decode as a confident "on battery, all off"
for a camera whose batLevel is the 8000 mains placeholder. Report
externalPower, sysMode and the funcBmp group as None in that case,
detected via the battery reading. 'lamp' deliberately stays 0 there --
consumers test for the key's presence to decide whether to offer a lamp
entity at all.
Also in the status block:
- p2pStatus is a count of attached sessions, not a status code. The app
renders it as "Connected: <n>" into a view named tvSessionNmb. Keeping
the upstream key name but no longer mapping a status enum onto it.
- recNmb and picNmb are the same all-ones non-answer with different
signedness (-1 vs 4294967295); both now decode to None.
- sdStatus and the Wi-Fi mode/security fields gain names.
Enums are named-only: they never change a decode, and an unrecognised
value yields None rather than the nearest name, because several of the
lists are incomplete. The two apps disagree on four DevType ids, so the
docstring carries both namings; ChipType exists in FtyCamPro only.
PTZ presets: verified against YsxLite's own call sites that SET(0)
stores and GET(1) recalls -- the scheme already in the code. REC(2)
takes index 0 and is a mode toggle, not a per-preset save. Every op was
then tried by hand against PTZA and FTYC and none of them does anything,
so presets stay documented as unsupported on the tested hardware.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A refused binary command answers with an *empty payload* and a negative LibError in the 4-byte token that parse_drw_pkt splits off the front. We only ever looked at the payload, so a refusal was indistinguishable from "answered with nothing" -- the command silently did nothing and the log said nothing about it. Add LibError (transcribed from YsxLite 1.40's own class, vendor typos included so the names stay greppable against the decompiled app), decode the token on every reply, and log a refusal at WARNING naming the command and the code. Failed logins now report why -- "CMD_EXCUTE_FAILED (-1010)" rather than an empty byte string. The per-command codes are kept in cmd_results so a caller whose wait_cmd_result came back empty can tell the two cases apart. The test UI's /info gains `auth` and a `refused` map, which is what turns "the panel is blank" into a list of which commands the camera turned down and why. Mapped on PTZA fw 2.2.15.93: only reboot (-1015 USER_NO_PRIVILEGE), wifi scan and the user list require a login. Video, audio, PTZ, lights, the status/info/datetime/wifi-settings reads, video params (get and set) and time sync all work unauthenticated -- so the non-fatal login is right. Note the firmware distinguishes UNAUTH (no session) from USER_NO_PRIVILEGE (session may not do that). binary_camera.py: fix a crash on any wrong password (odd-length hex string raised ValueError, so the simulator never answered USER_CHK at all and the client saw a setup timeout). Its "session ticket" 0e fc ff ff was itself a misread -- that is the little-endian -1010 from a failed login, copied from a capture. Replies now carry LibError.OK in the token on success, and the camera models the real permission split via auth_mode: 'normal' refuses only the three privileged commands, 'stuck' refuses everything with UNAUTH (a wedged camera, seen once and cleared by a power cycle), 'off' disables the checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Reopened. The decode bug that prompted the close is fixed, and the other two items from that comment have now been settled on hardware. The reason for closing — fixed.
Everything below was added since, and verified against PTZA and FTYC hardware:
One thing worth knowing when reading the diff: a camera can wedge itself into refusing everything with |
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a fairly large batch of work that came out of using
aioppppagainst tworeal binary-protocol cameras (PTZA and FTYC prefixes) over the last few
months. Everything here is device-verified unless noted; the README now carries a
per-device support matrix and a "Known issues" section.
Happy to split this into smaller PRs along the section headings below if you'd
prefer to review it in pieces — just say which order you want them in.
Correctness fixes
JsonSession.toggle_irraisedValueErroron every call.CancelledErrorwas swallowed infind_deviceand in the MJPEG stream handler,so client disconnects and shutdowns hung instead of unwinding.
VideoRotatewas never imported, soset_video_param('rotate', ...)failed.Session.stop()is now idempotent.set_resulttwice (or withNone).registered — a real race on fast LANs.
frame reassembly.
freeze on FTYC).
Connection lifecycle
P2pRdytimeout or session error no longer takes the process down.Devicegrew auto-reconnect with backoff, and anon_video_state_changecallback so consumers can reflect real streaming state.
--local-discovery-portto override.LanSearchExt(0x32) alongsideLanSearch(0x30) — some firmwares only answer the extended probe.
Performance
Binary protocol
The binary (non-JSON) side was largely stubs. It now covers:
BinaryCommandssplit into unambiguous enums (command and ACK previouslycollided on numeric values).
self-downgrade and ignore the value set at start time.
Date/time and status decoding
set_datetimesends the full 80-byte struct so the configured NTP serversurvives a clock write.
parse_datetime_blockhandles both firmware layouts observed (PTZA vs FTYC).store one.
rather than the app's four icon buckets.
Test tooling
Not part of the library, but this is what made the above verifiable:
control surface: parameter read-back, decoded info blocks, Wi-Fi scan and
device-users endpoints, a low-latency audio player, snapshot with a fallback to
the latest video frame.
multi-camera bugs findable.
binary_camera.py(the simulator) extended for end-to-end testing.proxy_camera.py: a transparent DID-rewriting proxy, useful for capturingvendor-app traffic against a real device.
Not covered / known limitations
touched only by the
toggle_irfix and shared session/video code, but asecond pair of eyes on that would be welcome.
and there is no UI for them yet.
they're probably only answered in AP/setup mode.
set_wifi's write layout doesn't match the confirmed read layout and isflagged do not use in the code — untouched by this PR, but worth knowing.
re-assert above is deliberate but unconditional). Documented under
"Known issues" in the README with a proposed fix; not implemented here
because it changes behaviour for snapshots.
(save=(1,0,n), goto=(1,1,n)); neither test camera acts on them. The API is
left in as best-effort for other firmwares.
Versioning
Numbered 0.3.0 (next minor after 0.2.3 on PyPI). Nothing has been published —
publishing is yours to do. Note that the companion PR
devbis/pppp_camera#14 pins
aiopppp==0.3.0, so that one needs this released (orthe pin adjusted) before it will install from HACS.