Fix silently-wrong camera state, plus vendored-engine parity - #102
Open
Fredde87 wants to merge 17 commits into
Open
Fix silently-wrong camera state, plus vendored-engine parity#102Fredde87 wants to merge 17 commits into
Fredde87 wants to merge 17 commits into
Conversation
`tutk/cuboai_pure.py` — the engine every LIVE surface runs on (streaming, talkback, and every IOCTL GET/SET behind the switches, selects, lights, numbers and the coordinator poll) — carried an older `transcode`/ `inv_transcode` that treated the partial-block tail as a plain XOR, with a docstring explicitly warning against re-adding the `Swap` permutation. That warning is wrong for the data channel. Native TUTK's TransCodePartial applies a `Swap` byte-permutation to the tail for tail lengths 2/4/8 (identity for every other length): encode `wire_tail = Swap(plain_tail XOR K16)`, decode `plain_tail = Swap(wire_tail) XOR K16`. So every data-channel frame whose length & 0xF is 2, 4 or 8 was built and parsed with a mangled tail. The 216-byte lullaby-schedule SET (tail 8) carries its duration there, and `build_set_sleep_safety_setting` (switch.py) likewise has fields in a Swap tail — both go out wrong today. Tails 6 and 12 (av-connect, the plain IOCTL GET) are identity, which is why the common paths always looked fine. The nuance the old docstring missed is that the exemption applies only to the pre-session SEARCH/broadcast frames: `build_probe`, `build_ack` and `build_lan_query` now pass `swap_tail=False` and keep the exact bytes they had, so session establishment is untouched — that was the "it breaks connect" fear, and the test pins those frames to prove it. This repo already ships the corrected implementation in `tutk/playback_engine/cuboai_pure.py` (verified byte-for-byte against the native library via ctypes, 2001/2001 tail-2/4/8 fixture datagrams — the old plain-XOR matched 0/2001). This commit brings the live copy to parity with it, and the new test uses that copy as an oracle so the two cannot drift apart again. tests/test_tail_swap.py: 7 checks — helper/involution, transcode and inv_transcode byte-for-byte against the oracle across lengths 0..96 plus the real frame sizes, round-trip symmetry, a guard that tails 2/4/8 actually changed, and the search-frame pin. Verified failing before this change (inv_transcode diverges at len=2) and passing after. (cherry picked from commit 793718d937e9c5a169cf19473bce842a4fc9e0f3) (cherry picked from commit a816af2) (cherry picked from commit 06da55f97f2b955dc450918867bf122818610b3b)
…x wrap The camera's reliable IO/control message-index (wire [56:58]) is a u16 that wraps 65535 -> 0. `_note_cam_data` advances `_data_ack` by contiguity over those indices, but `_data_ack` is an unbounded Python int — so the moment the index wraps, `(self._data_ack + 1) in self._cam_msgs` can never be true again and `_data_ack` FREEZES at 65535. The D field of every subsequent host->cam ACK ([40:42]) then pins at 0xFFFF and stops crediting the camera's send window. Nothing about the wire looks malformed while this happens, which is what makes it hard to spot from a capture: the frames keep flowing, one field just stops moving. The fix lifts each incoming index into `_data_ack`'s unbounded space with the `_unwrap_index` helper already in this file (the sibling `_idx_modular` fix sits directly above and does exactly this for the AV reassembly window), then discards consumed entries so `_cam_msgs` stays bounded and a stale wrapped value cannot false-advance a later epoch. Byte-identical below the wrap: `_unwrap_index` is the identity while no wrap has occurred, and `_data_ack` depends only on contiguity, which the discard does not change. Gated on CUBOAI_DATAACK_WRAP, default ON — a regression switch, not a tunable. `tutk/playback_engine/cuboai_pure.py` already carries this fix; this brings the live copy to parity. tests/test_dataack_wrap.py: 4 checks — ON crosses 65535 -> 65540 (wire 0xFFFF -> 0x0004), OFF freezes at 65535 (reproduces the bug, so the test has teeth), 399 pre-wrap steps identical ON vs OFF including a full build_data_ack frame, and _cam_msgs stays bounded over 5000 contiguous indices. (cherry picked from commit 887d99e9e5a7b0ee3cf1003a3484f8ff17d89f1b) (cherry picked from commit bfa8b5a) (cherry picked from commit 34496e605bbf452c1e7d8b0920846b95b21e5a45)
`send_audio` keeps every frame it has sent in `sent_buf`, keyed by `talk_frag`. `talk_frag` is deliberately monotonic and unbounded — it must not restart when a looped file wraps its content, or the camera rejects the replayed frames as already-seen. But the camera's 0x09 resend request names frames with a u16: `frag = (C + entry) & 0xFFFF`. Past 65536 frames — at 64 ms/frame, about 70 minutes of continuous or looping talkback — every `sent_buf.get(frag)` misses and talkback loss-recovery silently stops. Nothing errors and nothing on the wire is malformed; `resends_sent` simply stops rising, so audio quality degrades under loss with no signal anywhere that recovery has died. The fix lifts the u16 backward into `talk_frag`'s space with a new `_unwrap_index_back` helper (the mirror of the existing `_unwrap_index`, placed beside it). A resend request always names a frame we already sent, so the nearest congruent value at-or-below the current frag is the right one. Below the wrap it is the identity, so the wire is unchanged. Gated on CUBOAI_TALK_WRAP, default ON — a regression switch, not a tunable. `tutk/playback_engine/cuboai_pure.py` already carries this fix; this brings the live copy — the one the speaker/backchannel actually runs on — to parity. tests/test_talkback_resend_wrap.py: 6 checks — identity below the wrap, resolution across it, the at-or-below invariant, a modelled SACK decode where the legacy lookup resolves none of three requested frames and the fixed one resolves all three, the below-wrap no-change case, and a source pin so the shipped call site cannot drift away from the modelled arithmetic. (cherry picked from commit 3dc7832616721c298f5b18ee363354af9399d071) (cherry picked from commit 51d7ac5) (cherry picked from commit 4ed737deb06e41354a2e28a1efee1e4418c05e9c)
`_av_reader`'s `done_upto` is a per-reader local that starts at -1, but the camera's AV message-index is SESSION-scoped and keeps advancing. The FIRST read of a session therefore works, and any read that begins once the index is past the `done_upto + 256` accept window rejects EVERY fragment from that point on. The failure is silent, which is the dangerous part: fragments keep arriving at full rate, zero access units come out, and no error, gap counter or incomplete-AU counter moves. It looks like a camera that stopped sending. This is reachable through the legacy shim — `cuboai_transport_py.start_video()` opens a fresh `av_frames()` iterator on every call, so calling it twice on one session lands exactly here. No current Home Assistant path does that (the streamer runs one read per process and snapshots come from go2rtc's frame endpoint), so this is latent rather than a live failure today; it is still a trap sitting directly under a public method. The fix anchors the window at the reader's first accepted access-unit start. It fires only when the index is out of window, so a fresh-session stream is byte-identical — the test asserts that explicitly rather than assuming it. `tutk/playback_engine/cuboai_pure.py` already carries this fix (its DVR reader restores the live stream after playback and hits this on every restore); this brings the live copy to parity. tests/test_idx_seed.py: drives the real `_av_reader` over a real UDP socketpair with a synthetic camera. 5 checks — gate defaults ON; the in-window path is byte-identical ON vs OFF; starting at index 5000 emits ZERO AUs with the gate OFF (reproducing the bug) while fragments still arrive, and emits AUs with it ON; the mid-session output equals the fresh-session output; and a start just below the u16 wrap still works. Replay fixtures cannot cover this — they all start at index ~0, inside the window. (cherry picked from commit ee6f9533ce9393d4cdee9917ca5b88ccb5a80bef) (cherry picked from commit ef359bf) (cherry picked from commit 3f2d90ce608697a15b823d8208077f34fa6c7211)
`_AV_MID` — the 6-byte client fingerprint carried in the probe plaintext [58:64] and the AV/DATA plaintext [22:28] — is derived from the host NIC MAC. When every lookup fails, the old code returned a hard-coded `000000000000`, so every host in that state presented the camera with the same fingerprint. That state is not exotic for this integration: a network-isolated container with no NIC is a plausible Home Assistant deployment, and it is exactly where getifaddrs, /sys/class/net and uuid.getnode() can all come back empty. The camera does not validate this value's structure or origin, so this is hardening rather than a crash fix — a random 6-byte fallback works equally well and keeps distinct hosts distinct. The MAC-derived path is untouched, so on Linux and macOS behaviour is unchanged. Brings the live copy in line with tutk/playback_engine/cuboai_pure.py. tests/test_av_mid_fallback.py: 3 checks — the MAC-derived value is still stable, the fallback is 6 random bytes that differ between calls and is not the old all-zero constant, and the constant is gone from the source. (cherry picked from commit 3913b889ca955d4af85c37457c34b66b9673209a) (cherry picked from commit 222df28) (cherry picked from commit 7e8baeae16378f4be35dc366d9d56097ef628d2c)
… ignores `baby_presence_alert` is one of four coupled fields in SET_SLEEP_SAFETY_SETTING, and this camera firmware acknowledges writing it (result=0) without applying it. A live round-trip showed its read-back unchanged in both directions while `safety_alert`, `cover_alert` and `sensitivity` read back exactly as written in the same call — so the field is being ignored, not mis-encoded on the way out. The handler wrote the REQUESTED value straight into the coordinator cache and called async_write_ha_state(), so Home Assistant showed a state the camera was never in. The next poll silently reverted it, which reads as a flaky switch rather than as a camera that declined the command. For a baby monitor's presence alert, "Home Assistant says this alert is on when it is off" is the wrong failure to be quiet about. The helper now reads the value back inside the same session and returns what the camera actually reports; `_apply_verified` publishes that value and logs one clear warning per entity when it disagrees with the request. A failed read-back is treated as a failed read-back, not a failed SET — it falls back to the requested value and lets the next poll settle it, rather than inventing an unknown state. This does not try to make the field work; it cannot be made to work from the client side. It makes the integration honest about it. Removing or disabling the entity outright is a reasonable further step, but that is a breaking change for existing dashboards and automations, so it is left as your call. tests/test_set_readback.py: 6 checks — an honoured SET publishes and does not warn; an ignored SET publishes the camera's value rather than the request; the warning fires exactly once across repeated toggles; the helper returns the read-back rather than the requested value; and a read-back failure falls back to the request.
…g a native one `auto_discover_lib` was False in switch.py, light.py and the streamers, but left at its True default in coordinator.py, media_player.py, select.py, number.py and cuboai_stream_playback.py. Since `async_ensure_dependencies` downloads an optional `libIOTCAPIs_ALL.so` into `libs/<arch>/`, and that is one of the directories `_find_library()` searches, the integration can end up running the coordinator poll and some SETs on the native backend while the other SETs and all streaming run on the pure transport. Today that is masked by an incidental guard: `get_session` discards a discovered library whenever `camera_ip` is truthy. But the camera IP is auto-learned by the first successful poll, so a fresh install polls before it is known, and any setup where the IP stays unset keeps the split permanently. DVR playback is the sharpest case. `cuboai_stream_playback` does `inner = getattr(transport, "_inner", transport)` and hands the session to `PlaybackSession`, which drives the pure engine's RDT and channel-N paths. `_inner` exists only on `PureSession`, so a native session there degrades to reconnecting an already-connected transport and then driving a session that cannot do what is being asked of it. All five call sites now pass False, so the backend is a property of the integration rather than of whether a file happens to exist on disk. An explicit `lib_path` (or CUBOAI_LIB) still selects native for anyone who wants it, which is the documented way in. Worth considering separately: `async_ensure_dependencies` still pulls that .so from a third-party Docker Hub image, and runs `apk add gcompat` on Alpine, on every setup — both exist only to serve a backend nothing now selects by default. Removing that is a bigger product call, so it is left alone here. tests/test_backend_consistency.py: 2 checks — no call site auto-discovers a native library, and no `get_session` call omits the argument and inherits the True default (tokenised, so comments and docstrings mentioning get_session are not mistaken for call sites). Verified failing before this change, naming all five sites. Full suite: 277 passed. (cherry picked from commit 1716231a45ed9aaedf83d3194235a41b2840d73e) (cherry picked from commit 5c05dbf) (cherry picked from commit c1e5bbb95de0aac7afe64f3b7925139ab2da0bea)
`_generate_config` builds `config` from a fresh dict literal and `_write` opens
the file with "w", so go2rtc.yaml is fully regenerated on every call — nothing
is ever read back from it.
The comment above the stream block said the opposite ("this file is merged into,
not replaced", "keeping any other streams (e.g. from user)"), and a loop pruned
stale `cuboai_*` entries from `config["streams"]`. Because that key was created
empty two lines earlier and never populated from disk, the loop iterated an
empty dict and its body was unreachable.
Nothing was broken by it — regeneration already guarantees no stale entry
survives — but the comment claimed behaviour the code did not have, and the next
person to touch this would reasonably assume the merge existed. Replaced with a
direct assignment and a comment that describes what actually happens, including
why re-reading the file would be the wrong way to make the loop real: the file
lives inside the integration directory, and merging is exactly what would
resurrect the entries the loop was meant to drop.
tests/test_go2rtc_config_streams.py: 4 checks — the written stream set is
exactly the generated one; a stream that stops being generated does not survive
a regeneration (the outcome the loop was aiming at, asserted end to end rather
than through the dead branch); ports come from the resolved values; and NVR
credentials are written only when both are set. Full suite: 281 passed.
(cherry picked from commit 2ea829755dd29dd98f33905e484ad64f790dc2c8)
(cherry picked from commit e01c483)
(cherry picked from commit 83daed990cd675b8dece0452f58df7887d2d9b25)
…rforms The docstring said 'Any producer still running for a previous request is stopped first', while the body's own comment eight lines down explains at length why nothing is stopped: go2rtc's DELETE removes the declared stream rather than just its producer, which made every later request 404 until go2rtc restarted. Reading the summary and believing it would lead someone to conclude that a stale producer is impossible here, and to debug the wrong thing when a viewer gets the tail of a previous moment. Corrected to describe the actual behaviour and point at the reasoning already recorded below it. (cherry picked from commit cfbe523121962ffb62a13ecacc62cab247d6e1d7) (cherry picked from commit 3af47e0) (cherry picked from commit 088351052dc76af050811e2ff6bb4560af8bd91e)
…nection mode
`parse_session_stats` returns a dict with NO 'mode' key whenever `_extract_json`
cannot recover the embedded JSON from the response blob. The coordinator wrote
`stats.get("mode")` unconditionally, so that None went into `local_data`, and
the merge in `_fetch_all` — which exists precisely to carry values forward
across a failed read — overwrote the last good "lan" with nothing.
Every other failure path in `_fetch_local_data` leaves its key unset and is
carried forward. This one escaped that protection because the call did not
raise: it succeeded and simply had nothing in it.
Measured on a live camera over 12 hours (recorder database, 60s poll interval):
the Connection Mode sensor flapped to `unknown` 200 times, each spell lasting a
median of 64.6s — exactly one poll — before recovering, for a value that never
once actually changed from "lan". Roughly one poll in three. That makes the
sensor useless for automations and writes several hundred pointless rows a day
into the recorder.
The mode is now only written when the response actually carried one, and the
miss is logged with the response length so the underlying parse failure stays
diagnosable. "Unknown" was never a true answer here in any case: this GET only
returns at all because the local session to the camera is up.
Also fixes the test harness this needed. conftest MagicMocks `homeassistant`,
which makes it a non-package, so every submodule a platform module imports has
to be registered explicitly — and the entity bases must be real, distinct
classes, with `CoordinatorEntity.__init__` actually assigning `self.coordinator`
as the real base does. Those stubs now live in conftest instead of in individual
test modules, where whichever module pytest imported first silently decided what
every later one saw.
tests/test_connection_mode_carry.py: 5 checks — a present mode is reported; a
response with no mode, and one with mode=None, both leave the key ABSENT rather
than None; the pre-existing raised-exception path is pinned alongside them; and
the merge contract itself (an absent key keeps its previous value) is asserted
directly. Verified failing against the previous coordinator (2/5) and passing
after. Full suite: 286 passed, order-independent.
(cherry picked from commit 1e581c4188ac478fd68009ba6fd59d2e46a8a5f2)
(cherry picked from commit ed83b5e)
(cherry picked from commit 78f6b3b9611dc01f7a6e9aa5589de621221ff17e)
…eat it
`_extract_json` pulled the JSON body out of a binary IOCTL response with a
greedy `\{.*\}`, which spans from the FIRST '{' to the LAST '}' anywhere in the
blob. Responses like GET_SESSION_STATS (0x0935) and GET_USER_LIST (0x0947) carry
counters and padding after the object, so a single stray 0x7D byte among them
swallowed everything past the real object, json.loads failed, and the entire
response was discarded.
Because those trailing bytes are counters, whether a 0x7D landed there varied
from poll to poll — which is why this presented as an intermittent, self-healing
parse failure rather than a consistent one, and why it went unnoticed. On a live
camera it showed up as the Connection Mode sensor dropping to `unknown` for
exactly one poll and recovering: 200 times over 12 hours, roughly one poll in
three. The preceding commit stops that None erasing the good value; this removes
the cause.
Extraction is now brace-counted and string-aware, so a brace inside a string
value or a nested object is handled correctly, and it stops at the end of the
first complete object. The greedy match is kept as a fallback, so this can only
ever parse more responses than before, never fewer. A genuinely truncated body
still returns None, which is the honest answer.
Note this function is vendored from the pure-transport tree, so the same
weakness exists upstream in cuboai_messages.py and is worth carrying back.
tests/test_extract_json.py: 10 checks — a plain embedded object still parses with
the bare dotted-quad IP repair intact; trailing binary containing a stray 0x7D is
recovered (and the old greedy implementation is asserted to FAIL on that same
input, so the test pins the actual bug); trailing binary without one is
unaffected; braces inside strings, escaped quotes and nested objects are all
handled; the first object wins when a blob holds two; truncated and JSON-free
blobs return None; and parse_session_stats recovers the mode end to end.
Full suite: 296 passed.
(cherry picked from commit 4674a5be49d2bc083c76e39dae7ff365aa293197)
(cherry picked from commit 273a94a)
(cherry picked from commit 59e937194e8f6fa95e5cef211b5283cb26c34631)
… guess
The Lullaby Timer number returned a purely local value. It started at a
hardcoded 30, was restored across reloads, and was never reconciled with the
camera — so a lullaby the camera was repeating indefinitely still showed as
"30 min" in Home Assistant. It was a write-only setpoint presented as a state.
The camera's actual timer was already being fetched and thrown away. The
coordinator calls `get_lullaby_schedule()` for the volume, and the same response
carries `timer_mode` at offset 8 (0 = repeat forever, 1800 = 30 min,
3600 = 60 min) plus the `timer_name` the parser already derives. It now surfaces
all three, and the entity reads them. A value the user picks is held as
`pending` so it survives the next poll, and clears once the camera reports it
back; the camera's own reading and a `differs_from_camera` flag are exposed as
attributes.
Also fixes three fabricated values feeding the same code path:
* The coordinator wrote `lullaby_volume = 50` whenever the schedule read
failed. The merge in `_fetch_all` then treated that constant as a real
reading and carried it forward, so a failed read silently reported the wrong
volume rather than keeping the last known one. It now leaves the key unset,
like every other failure path here.
* `_push_native_timer` defaulted the volume to 50, and the "volume" command
defaulted the timer to repeat-forever. SET_LULLABY_VOL_DURATION is a COUPLED
write — one struct carries both fields — so supplying only one and inventing
the other silently changed the field the user had not touched: adjusting the
volume cancelled the sleep timer, and adjusting the timer reset the volume to
50. Both now read the camera's current pair and modify only the field being
changed, falling back to a constant solely when that read also fails, with a
log line when it does.
Note the write side was already correct: minutes are converted to seconds
(`timer * 60`) before the SET, matching the camera's 0/1800/3600 encoding.
tests/test_lullaby_timer.py: 11 checks — the camera's repeat and 30-minute
timers are reported (the reported symptom asserted directly); the local value is
used only when the camera is silent; a fresh pick survives a poll and clears when
the camera agrees, after which the camera wins again; setting volume alone
preserves the camera's timer and setting the timer alone preserves its volume;
0 minutes still means repeat; both-supplied is honoured; and an unreadable
schedule still writes something sane. Verified failing against the previous code
(6/11) and passing after.
conftest gains the number/media_player/entity_registry/dispatcher stubs these
platforms need, alongside the ones added earlier. Full suite: 307 passed.
(cherry picked from commit 5e0f6bf43ce744731c5e6495a400c7df2a6af4a9)
(cherry picked from commit 80d8ac5)
(cherry picked from commit 6414f2b3aa49a46c39cd16778243f093a5ddfa77)
Both callers of the "play" command pass volume=None — `async_media_play` and the card path through `async_select_source` — and the play branch fell back to a hardcoded 50. Since SET_LULLABY_VOL_DURATION writes volume and timer in one struct and is sent BEFORE the play, starting any lullaby silently overrode whatever volume the user or the CuboAi app had set. "Not specified" now means "keep the camera's", the same rule the volume command already follows after the previous commit: the current volume is read in-session and only falls back to 50 if that read also fails, with a log line when it does. The timer semantics of this branch are deliberately unchanged and now say so: timer=None means repeat-forever because the card path lets Home Assistant enforce the duration and send the stop itself. tests/test_lullaby_timer.py gains 4 checks — play keeps the camera's volume when none is supplied (the bug, asserted as 50 != 70 against the old code), honours an explicit volume, still means repeat-forever with no timer, and converts minutes to the camera's seconds encoding. Full suite: 311 passed. (cherry picked from commit 8ae6e17612b9e911893e21c3ca1187dd31a4b655) (cherry picked from commit 143426b) (cherry picked from commit 9625235d335c14fed0983d709929b1c2761f5727)
…e coupled write The camera has exactly one write for the lullaby volume and sleep timer: SET_LULLABY_VOL_DURATION (2438), a 140-byte struct carrying id@0, duration@4 and volume@8 with no field mask. There is no volume-only or timer-only opcode — 2434 is play/stop and 0x0990 is the alarm-clock schedule table — so every write necessarily carries both fields and must be a read-modify-write. `cuboai_messages` already ships exactly that: `build_set_lullaby_schedule(volume, duration, get_resp_bytes)` takes the raw GET_LULLABY_SCHEDULE echo and preserves whichever field is left as None. The previous commit hand-rolled the same logic in two branches of `_execute_lullaby_cmd`; this replaces both with the library helper so there is one implementation of the rule rather than three, and the "why" is documented where the echo is fetched. Behaviour is unchanged from the previous commit — volume=None and timer=None still mean "keep the camera's", 0 minutes still means repeat forever, and the card path still deliberately asks for repeat so Home Assistant can send the stop. The tests are stronger for it: instead of mocking the builder, they now decode the REAL 140-byte payload the REAL builder produces off a mocked ioctl, so they cover the wire layout (timer@4, volume@8) and the minutes-to-seconds conversion as well as the preserve-the-other-field rule. 17 checks, 4 of them failing against upstream's code — including `assert 1 == 70`, upstream sending a MagicMock-derived volume where the camera's own 70 should have been preserved. Full suite: 313 passed. (cherry picked from commit adb53fa4e00850a143008adf1a9d7d64dae6e7da) (cherry picked from commit 88364d5) (cherry picked from commit 085cb653b6adcff18650198a64b41427b9e84d3b)
… constant
`local["brightness"]` was read in two places and written in none. Nothing in the
integration called GET_LIGHT_STYLE, so the key never existed:
* `CuboNightLightBrightnessNumber.native_value` fell through to its hardcoded
fallback of 100 on every poll, no matter what the camera was set to
(observed live: the entity sat at exactly 100);
* `CuboNightLight.brightness` returned None while the entity advertises
ColorMode.BRIGHTNESS, so the light had no brightness to show either.
Setting the brightness worked — it writes straight to the camera — so this only
showed up as the displayed value never moving, and snapping back after a reload.
The camera reports it on GET_LIGHT_STYLE at offset 24 as a percent; the builder
and parser were already vendored here, just never called. The coordinator now
reads it alongside the other hardware GETs, and a failed read leaves the key
unset so the merge carries the last known value forward, like every other read
here. The number reports unknown rather than a constant when there is genuinely
no value.
Found by sweeping every field each parser returns against what the integration
actually consumes. Same shape as the lullaby-timer defect: an entity presenting a
local guess as if it were camera state.
tests/test_night_light_brightness.py: 6 checks — the poll reports the camera's
value verbatim (two brightnesses), a failed read leaves the key unset, the entity
reports the camera's value, reports unknown instead of a hardcoded 100, and does
not confuse a genuine 100 with the old fallback. Verified failing against the
previous code (3/6) and passing after. Full suite: 319 passed.
(cherry picked from commit ba702aa73f5584efada744fee11574dde5ef9165)
(cherry picked from commit 0445a7c)
(cherry picked from commit efd21d6b27af7bf1b3641b9208e6d0c0d63c9554)
…ites
The integration passes camera state around as a plain dict at
`coordinator.data["cameras"][id]["local"]`, and nothing checked that the key an
entity reads is the key the coordinator writes. Sweeping every field each parser
returns against what the integration consumes turned up three mismatches:
* The Status LED switch read `status_led_on`. The poll writes
`status_light_on` (from `get_hw_control().status_light_on_off`), which
nothing read. The switch therefore never reflected the camera — it showed
only what its own optimistic update had written, and returned to False after
every restart. All three sites (the read and the two optimistic writes) now
use the polled name, so the switch converges on the camera within one poll.
This is a wiring fix only; it makes no claim about whether the LED itself
responds to a SET.
* The Sleep Safety sensor exposed a `raw_value` attribute reading
`sleep_safety_raw`, which nothing ever wrote, so it was always None. The
camera actually distinguishes "Covered Face and Rollover Alerts" from
"Covered Face Only", and `parse_sleep_safety_setting` already returns that
as `mode`/`mode_desc` — the sensor's On/Off was hiding it. The dead
attribute is replaced by the real mode.
* `lullaby_timer_mode`, which the previous commit surfaced, is dropped: the
minutes and the name derived from it are both exposed and read, and a value
written but never read is the same cruft in the other direction.
tests/test_local_keys_reconcile.py is the guard for the whole class: it
re-derives the read set and the poll-written set from the source and fails on any
key read by an entity that the poll never writes. Deliberately, an optimistic
post-SET update does NOT count as a write — that is precisely how
`status_led_on` hid, since the switch both wrote and read it while never
reconciling with the camera. A second check asserts the patterns still match a
plausible number of keys, so the guard cannot pass vacuously if the source style
drifts.
tests/test_night_light_brightness.py gains 2 checks for the Status LED switch
reading the polled value.
The one remaining asymmetry is `camera_angle`, which the poll writes and no
entity reads — harmless, and left alone rather than guessing at an entity for it.
…ys called `coordinator.py` called `client.get_lullaby_schedule()`. That method was never defined on `CuboAIClient`. Every poll therefore raised AttributeError, the surrounding `except Exception` swallowed it, and the old hardcoded `data["lullaby_volume"] = 50` fallback made the result look like a real reading. So the lullaby volume Home Assistant displayed was never once the camera's — it was the constant 50, and a genuine volume of 50 is indistinguishable from it, which is why it went unnoticed. Removing the fabricated fallback in an earlier commit did not help on its own: the value simply became absent, and the media player's own display fallback still rendered 0.5. `GET_LULLABY_SCHEDULE` (2440) is the ONLY readable source for the lullaby volume and sleep timer — GET_LULLABY_VOL_DURATION (2436) carries the song and play state but not the volume, which `parse_lullaby` documents with an explicit `'volume': None`. The builder, the response constant and `parse_lullaby_schedule` were all already present and registered in GET_METHODS; only the client wrapper was missing. The coordinator now consumes `parse_lullaby_schedule`'s dict — the authoritative parser, which reads volume @12 (live-confirmed) and deliberately does not surface @16 as a play flag because it reads 0 while sound is actually playing. This also populates the timer, so the Lullaby Timer entity added earlier finally has a camera value to reconcile against instead of a null. tests/test_client_methods_exist.py is the guard for the class: it re-derives every `client.<method>()` call in the integration and every method defined on CuboAIClient, and fails on any call with no definition. A missing method is otherwise invisible here, because nearly every camera read sits inside a broad except — this one survived for as long as the fallback looked plausible. Plus a vacuity check on the pattern, and two behavioural tests of the new method (volume and timer decoded off a synthetic response; a mismatched response type raises). Verified failing against the previous commit, naming the exact call. Full suite: 340 passed. (cherry picked from commit 3ceac41484f4890ea16bf2fe17ef36d91337b145) (cherry picked from commit 4c0ffad) (cherry picked from commit 859e1857217bd667e71a42adc3e95262691b142a) (cherry picked from commit 5d4dad3c5e6ccb5d98bcff18fdf855e804cf421a)
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.
I've been running this integration for a while. I went looking for why the lullaby volume never matched my camera and found a cluster of related problems. 17 commits, each with tests, all measured on a live Cubo 3 camera.
The common shape: almost every camera read sits inside a broad except, and several have a plausible-looking constant as a fallback. When a read fails, the constant is indistinguishable from a real reading, so the failure is invisible, sometimes for a very long time.
Silent read failures
Coupled writes
SET_LULLABY_VOL_DURATION carries volume and timer in one struct, with no volume-only or timer-only opcode, so every write is necessarily a read-modify-write. Several call sites supplied one field and defaulted the other, so starting a lullaby reset the volume to 50, and changing the volume cancelled a running sleep timer. build_set_lullaby_schedule already existed as the RMW helper; the call sites now use it.
Vendored-engine parity
tutk/ had drifted behind tutk/playback_engine/. Backported: the TransCodePartial tail Swap (mis-encodes any frame whose length mod 16 is 2/4/8 — affects the lullaby-schedule and sleep-safety SETs), two u16 wrap fixes, the mid-session reassembly seed, and a random AV-MID fallback.