Skip to content

Report state on models whose property map spans more than one frame - #5

Open
pentafive wants to merge 4 commits into
kugaevsky:mainfrom
pentafive:fix/g8-split-property-map
Open

Report state on models whose property map spans more than one frame#5
pentafive wants to merge 4 commits into
kugaevsky:mainfrom
pentafive:fix/g8-split-property-map

Conversation

@pentafive

Copy link
Copy Markdown

Summary

Found running the integration on two Glowrium G8s (pkey Glowrium-C064, firmware 2). Every state entity sat at unknown while the lamps took commands normally. Fixing that turned up three further correctness problems in the same area. Four commits, each self-contained and green on its own, so any can be dropped.

1. A property map split across frames is discarded entirely. The G8's state notification declares more CBOR pairs than the frame carries. Mine is 55 bytes with a map header of 0xac, so it promises 12 pairs and contains 11:

ac 06f5 08 1846 09f4 0a fb4043747ae147ae14 0b fb40534147ae147ae1
   0df4 11 4b 010200ff0a12121264000014f5 17f5 182b01 182f420000

cbor.decode() runs out of buffer on the twelfth pair and raises IndexError, _on_notify catches it, and the whole frame goes, discarding eleven valid properties: power, brightness, circadian, lat/lon, schedule, timer slot, activated, indicator, lighting mode and ramp. It logs at debug, so without debug logging there is no visible symptom at all.

I cannot tell from outside whether the device fragments or truncates. Either way the frame that arrives is usable. Nothing here looks G8 specific; my guess is the G7 never splits a frame, so the path has not been exercised. _map() now stops at the buffer boundary and decode_frame() reports it via a short flag. decode() stays strict, since it only round-trips payloads we encoded ourselves.

2. State is primed by reading facebd02 rather than requesting it. The characteristic is readable and one read returns 20 keys in a single response, which is cheaper and not exposed to the splitting above. The batched request stays as a fallback but is sent at most once per coordinator.

That matters beyond the G8: the request write is unreliable there, answering ATT Insufficient authorization (0x08), a not-connected error, or a 30 second timeout depending on route and timing, and dropping the link each time. Since _connect_locked also runs from the command path, it was tearing the connection down on every command, so a brightness change only landed if it won the race.

The read returns more keys than STATE_KEYS asked for, so state now holds properties with no names, on a G7 too. I checked this is inert: every access is a keyed lookup, nothing iterates or serialises the dict, and STATE_KEYS only builds the request payload.

3. Schedule and mode writes no longer invent fields they never read. editable_timer_slot() fell back to TIMER_DEFAULT when 0x11 was unread. The slot packs the enabled flag, both times, brightness and fade into one write, so changing one field through that fallback silently overwrote the other four and forced enabled=1. It now returns None and the setters raise HomeAssistantError. Same guard where _mode_payload would default the lighting mode to index 1.

I left the ramp fallback alone. Setting a mode rewrites ramp on the device regardless, so there is no leave-it-alone option and RAMP_DEFAULT is a real decision. I had guarded it too at first, which broke your capture-verified test_set_lighting_mode_matches_capture. That test was right and I was wrong.

4. light.is_on reports unknown rather than off. It returned bool(state.get(KEY_POWER)), and bool(None) is False, which renders as a definite off. An unread lamp claimed to be off while physically on, and automations, scenes and light.toggle reasoned from that. Now returns None, matching switch.py.

Existing tests this changes

Three of your tests asserted the old defaulting, so they had to change. Flagging rather than leaving it to review:

  • test_editable_timer_slot_is_an_independent_copy asserted editable_timer_slot({}) == bytearray(TIMER_DEFAULT). It now asserts None for an unread slot, and still asserts the independent-copy property for a read one.
  • test_set_timer_start and test_set_timer_gradual called the setters against empty state and now seed KEY_TIMER first. test_set_timer_start's own docstring says it "edits only the start bytes of the 0x11 slot", which is what it does now and is not what it did before.

If you disagree with the reasoning, commit 3 is the one to drop.

Known limitations

0x17 (indicator) and 0x35 (DST) are outside the range the read returns, so on a G8 those two entities stay unknown until something writes them. They remain writable and the optimistic echo populates them then, so they are usable but effectively write-only.

Since 0x2b is also absent from the read on a G8, async_set_ramp should refuse there rather than silently reset the mode. I could not confirm that on hardware because the ramp entity is unavailable outside Circadian, so treat it as reasoning rather than observation.

I have not touched models.py. Per CONTRIBUTING that needs the preset indices confirmed against a btsnoop capture, and I do not have the vendor app paired to these lamps. Guessing would be worse than the current generic fallback.

The new HomeAssistantError messages are plain strings rather than translation_key entries with a strings.json exceptions: block. Happy to convert them and add the six translation files if you would prefer that.

Related

I have opened two issues so nothing depends on this PR:

Type of change

  • Bug fix
  • New feature / entity
  • New device model (models.py)
  • Docs / chore

Checklist

  • ruff check . and ruff format --check . pass. ruff check . passes repo-wide. ruff format --check . reports one file, ARCHITECTURE.md, which already fails on main independently of this change; everything this PR touches is clean.
  • pytest passes. 61 passing, and each of the four commits is green on its own.
  • strings.json and every translations/*.json stay key-for-key in sync (if UI strings changed). Untouched, though see the note above about exception messages not using translation keys.
  • manifest.json key order follows hassfest; version bumped if this is a release. Untouched, and not a release.
  • Tested on hardware, or explicitly noted as untested. Tested on two G8s, with the split below.

What was exercised on hardware: the split-map decode, priming from the read, and the light reporting its state. These took the lamps from every entity unknown to correct values. Brightness round-trips were checked against the power meters in the smart plugs the lamps sit on rather than the integration's own numbers: 20% drew 2.94 W, 35% 4.61 W, 45% 5.8 W, 99% 11.26 W, near enough linear at about 0.105 W per percent. No errors from the integration in the log across several restarts.

What was not: the refusal paths in commit 3, and trailing-byte rejection. The refusals cannot be reached on my lamps precisely because the fix works. 0x11 now reads, so the guard never fires, and the schedule and ramp entities are mode-gated to unavailable while both lamps sit in Manual. Unit tests only.

The G7 path is covered by tests but I have no G7. A G7 that cannot be read still gets the request exactly as before.

Two problems in the CBOR codec, both reachable from untrusted device input.

A property map split across frames was discarded whole. A Glowrium G8 sends a
state notification declaring 12 pairs but carrying 11, so decode() raised
IndexError and the caller's handler binned the frame along with the eleven valid
properties it did contain. _map() now decodes pair by pair and stops at the
buffer boundary, and decode_frame() reports that via a short flag. decode()
itself stays strict: it only round-trips payloads we encoded, where a short map
really is a bug, so only the device path opts into tolerance.

Neither entry point checked the buffer was consumed, so a frame carrying extra
bytes decoded to a plausible short map and ignored the rest. That matters
because {0x14: False} is the value that makes the coordinator replay the vendor
bring-up sequence. Trailing bytes now raise.
NOTIFY_UUID is readable and one read returns the whole property map in a single
response, which is cheaper than a request plus notification and not exposed to a
map being split across frames.

The batched request is kept as a fallback for devices that cannot be read, but
is now sent at most once per coordinator. On a G8 that write is unreliable,
answering ATT 0x08, a not-connected error or a timeout depending on route and
timing, and dropping the link each time. Since _connect_locked also runs from
the command path it was tearing the connection down on every command, so a
brightness change only landed if it won the race against the disconnect.

Decoding and merging moves into _ingest(), shared by the notify callback and the
connect-time read so both handle a split map, ramp seeding and listener
notification identically. Device-info is read before the state request, so a
device that drops the link on that request can still be named in the warning.
The activation wait is skipped when state is known unreadable, where it
previously burned three seconds on every connect waiting for a flag that could
not arrive, and a device whose activation flag cannot be read is never activated
blind.

Ramp seeding now guards on truthiness rather than is-not-None, so a zero length
ramp no longer latches and blocks re-seeding permanently.
editable_timer_slot() fell back to TIMER_DEFAULT when 0x11 had never been read.
The slot packs the enabled flag, both times, brightness and the fade into one
write, so changing a single field through that fallback silently overwrote the
other four with values the user never chose, and forced enabled=1. It now
returns None and the four setters raise HomeAssistantError explaining why.

Same guard where _mode_payload would otherwise default the lighting mode to
index 1, silently changing a setting the caller never asked to touch.

The ramp fallback is deliberately left alone: setting a mode rewrites ramp on
the device regardless, so there is no leave-it-alone option and RAMP_DEFAULT is
a real decision rather than an accident.

Three existing tests asserted the old defaulting and are updated. Note that
test_set_timer_start's docstring already described the new behaviour, saying it
edits only the start bytes of the slot.
is_on returned bool(state.get(KEY_POWER)), and bool(None) is False, which Home
Assistant renders as a definite off. A lamp whose state had not been read yet
therefore claimed to be off while it was physically on, and automations, scenes
and light.toggle all reasoned from that. Returning None renders as unknown
instead, matching how switch.py already handles the same situation.
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.

1 participant