Skip to content

feat: add Universal Remote Control support - #520

Open
Onero-testdev wants to merge 7 commits into
sblibs:mainfrom
Onero-testdev:feat/universal-remote
Open

feat: add Universal Remote Control support#520
Onero-testdev wants to merge 7 commits into
sblibs:mainfrom
Onero-testdev:feat/universal-remote

Conversation

@Onero-testdev

@Onero-testdev Onero-testdev commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Add support for the SwitchBot Universal Remote Control.

This is a new model that exposes its battery level and charging state.

Details

Advertisement parsing

  • New parser in adv_parsers/remote.py.
  • Battery and charging state are in manufacturer data. ADV byte 14 maps to mfr_data[7]:
    • bit 7 -> charging state (0 = not charging, 1 = charging)
    • bits 6-0 -> battery level (1-100%)

Device class

  • New SwitchbotUniversalRemote device (devices/universal_remote.py).

Note from SwitchBot team: This PR was developed by the official SwitchBot team and has been verified with real hardware testing. For questions, contact Discord @7eaves / GitHub @fankai777.

@codecov

codecov Bot commented Jun 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
switchbot/__init__.py 100.00% <100.00%> (ø)
switchbot/adv_parser.py 97.82% <100.00%> (ø)
switchbot/adv_parsers/remote.py 100.00% <100.00%> (ø)
switchbot/const/__init__.py 100.00% <100.00%> (ø)
switchbot/devices/device.py 69.90% <ø> (ø)
switchbot/devices/universal_remote.py 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Onero-testdev
Onero-testdev marked this pull request as draft June 17, 2026 03:08
@bluetoothbot

bluetoothbot commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

Previous review — superseded by a newer review below.

bluetoothbot
bluetoothbot previously approved these changes Jun 20, 2026

@bluetoothbot bluetoothbot 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.

No blocking issues found.

@Onero-testdev
Onero-testdev marked this pull request as ready for review July 3, 2026 06:35
bdraco
bdraco previously approved these changes Jul 9, 2026
@bdraco

bdraco commented Jul 9, 2026

Copy link
Copy Markdown
Member

Looks like pre-commit is failing

@Onero-testdev
Onero-testdev dismissed stale reviews from bdraco and bluetoothbot via 6d1c9c1 July 9, 2026 07:22
@Onero-testdev Onero-testdev changed the title feat: add Remote With Screen (Universal Remote) support feat: add Universal Remote Control support Jul 9, 2026
@Onero-testdev
Onero-testdev force-pushed the feat/universal-remote branch from 9a4c644 to 8643677 Compare July 9, 2026 08:38
Onero-testdev and others added 5 commits August 10, 2026 18:39
Add support for the SwitchBot Universal Remote (Remote With Screen,
WoRemoteWithScreen). The device advertises its battery level and charging
state in the manufacturer specific data, and the battery level / charging
state can also be queried over BLE via the basic info command.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Align with the official product name. Renames the device module,
class, model enum, adv parser function, and all references/tests
from remote_with_screen/RemoteWithScreen to universal_remote/
UniversalRemote. The model enum value becomes "WoUniversalRemote".

Hardware byte-pattern identifiers are unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Onero-testdev
Onero-testdev force-pushed the feat/universal-remote branch from d1ac8c4 to a5596cd Compare August 10, 2026 11:06
@bluetoothbot

Copy link
Copy Markdown
Collaborator

@bdraco: Looks like pre-commit is failing

Reproduced locally. tests/test_universal_remote.py lacks a trailing newline.

Trips two hooks: end-of-file-fixer and ruff-format. Confirmed via ruff format --diff against this repo's config — reports \ No newline at end of file.

Ran ruff check on both new source files. Both clean. So this newline looks like the sole cause.

Fix: add newline, then pre-commit run --all-files.

Caveat: reviewed from diff, not a branch checkout — could not byte-check.

@bluetoothbot

bluetoothbot commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Previous review — superseded by a newer review below.

@bluetoothbot bluetoothbot 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.

Warning

Important issues found.

  • Missing trailing newline — fails pre-commit (end-of-file-fixer + ruff-format)

@Onero-testdev

Copy link
Copy Markdown
Contributor Author

Addressed the remaining review feedback in 342593f:

  • Removed the contradictory absolute ADV-byte mapping and retained the precise mfr_data[7] bit layout.
  • Verified tests/test_universal_remote.py already ends with a newline on the current branch.

Validation: all pre-commit hooks pass, the full local suite passes (1312 passed), and all GitHub checks are green.

@bdraco ready for re-review when convenient. Thanks!

@bluetoothbot

Copy link
Copy Markdown
Collaborator

@Onero-testdev: Addressed the remaining review feedback in 342593f:

Both confirmed resolved. Docstring now single-line plus bit-layout comment — matches AGENTS.md. Newline: diff shows no \ No newline at end of file marker, so file ends correctly. Blocker dropped; approving.

Six suggestions remain, none blocking. Highest value: rename charging to battery_charging. Precedent — art_frame.py:25, keypad_vision.py:26. Same mfr_data[7] bit, different name. Cheap now, breaking after release.

Also: b"\x07" not "\x07", and restore firmware in get_basic_info. Seventeen of eighteen device modules return it.

@bluetoothbot

bluetoothbot commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Previous review — superseded by a newer review below.

@bluetoothbot bluetoothbot 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.

Tip

No blocking issues found — ready to merge.

@bdraco

bdraco commented Aug 19, 2026

Copy link
Copy Markdown
Member

Bot left some suggestion, but still needs human review, and out of time.

@bluetoothbot

Copy link
Copy Markdown
Collaborator

@bdraco: Bot left some suggestion, but still needs human review, and out of time.

One item worth your eye before merge. get_basic_info reads _data[1] unmasked, charging from separate _data[12]. Every sibling packs charging into bit 7 of battery byte: art_frame.py:30-31, keypad_vision.py:51, fan.py:205. This PR's own parser masks. Device class does not. If layout matches siblings, battery reports 128-228 while docked. Masking costs nothing either way. Tests cannot catch it — fixtures written from code. Rest stay suggestions: charging vs battery_charging naming, dropped firmware, "\x07" as str not bytes, untested \x07 branch.

@bluetoothbot

Copy link
Copy Markdown
Collaborator

PR Review — feat: add Universal Remote Control support

Solid, well-registered device addition — one byte-layout inconsistency in the device class is worth resolving before merge.

Specific things done right:

  • Both prior blockers stay cleared. The trailing-newline issue and the self-contradictory docstring are gone; the parser now carries the one-line """Process Universal Remote adv data.""" plus a single inline comment for the bit layout, exactly the shape AGENTS.md asks for.
  • The adv parser is byte-for-byte right. I re-read art_frame.py:16-17 and keypad_vision.py:14-15 this pass — mfr_data[7] with & 0b01111111 for battery and bit 7 for charging is precisely what the 2409 family does.
  • Guard sized to max_index + 1. len(mfr_data) < 8 covers the read at index 7, and it is wired into the existing test_short_payload_guards.py suite rather than a one-off.
  • Discovery done the right way. Service-data first-byte discriminators, no manufacturer_data_length hint. I verified against every key in SUPPORTED_TYPES that neither \x07 nor ' collides, and extracted every service_data fixture in test_adv_parser.py to confirm no existing suffix-matched device leads with a 0x07 first byte that the new short-circuit would hijack.
  • Registration is complete: const, API_MODEL_TO_ENUM, package export (correctly isort/alphabetical), SUPPORTED_TYPES, device class, tests.

What needs attention:

  • 🟡 get_basic_info reads _data[1] unmasked and takes charging from _data[12] — the PR's own parser masks that byte, and art_frame.py:30-31, keypad_vision.py:51, fan.py:205 all pack charging into bit 7 of the battery byte. If the basic-info reply follows suit, battery reports 128–228 while docked. Masking is free and safe either way; the tests can't adjudicate because they were written from the code.
  • 🟢 charging vs the battery_charging key used by the four other places that decode this bit — free now, breaking after release
  • 🟢 firmware dropped from the base-class contract; 17 of 18 device modules return it
  • 🟢 _data[12] read with no length guard — relay_switch.py:139 is the guarded precedent, added after issue Single data byte causing index out of range #369
  • 🟢 "\x07" registered as str where all ~25 comparable control-byte keys are bytes, and listed before "'", which makes the explicit-model path report the pairing byte
  • 🟢 The \x07 dispatch branch is never exercised by a test and isn't mentioned in the PR body
  • 🟢 "WoUniversalRemote" as the cloud deviceType is worth a one-line confirmation; recent additions all use W1xxxxxx codes

Not independently verifiable from here: the hardware byte offsets in the basic-info reply. The advertisement offsets, by contrast, have five in-repo precedents and are safe.


🟢 Suggestions

1. Key name `charging` diverges from `battery_charging` used by the two closest precedents (carried over — still open)
switchbot/adv_parsers/remote.py:41

The two existing parsers that decode this exact bit — bit 7 of mfr_data[7] on a 2409 device — both publish it as battery_charging:

  • adv_parsers/art_frame.py:16,25
  • adv_parsers/keypad_vision.py:14,26

The only "charging" key in the library is adv_parsers/fan.py:47, which reads a different byte on a different (741-family) layout and backs the SwitchbotFan.charging property. Repo-wide the count is 4 × battery_charging vs 1 × charging.

Why it matters: these dict keys are the library's public surface — Home Assistant maps them onto entities. Two names for one concept forces a per-model special case for the Universal Remote instead of reusing whatever already handles Art Frame / Keypad Vision. Free to fix now, breaking once released.

Rename in the parser, in SwitchbotUniversalRemote.get_basic_info, and in the four tests that assert on it.

        "charging": bool((mfr_data[7] >> 7) & 1),
2. No length guard before `_data[12]` (carried over — still open)
switchbot/devices/universal_remote.py:22

_get_basic_info (device.py:793-803) only rejects the two single-byte error replies b"\x07" / b"\x00"; a truncated-but-longer payload passes straight through and _data[12] then raises IndexError out of update().

Held at suggestion because several siblings are equally unguarded (bot.py:105-111 reads _data[10]), but note the precedent that does guard is the most recently swept one — relay_switch.py:139-143 explicitly does if len(_data) < 17: _LOGGER.warning(...), added after issue #369 produced a live stack trace from exactly this shape (a \x02 payload reaching a device-level parser). Index 12 is the highest unguarded read in devices/.

One line closes it:

if not (_data := await self._get_basic_info()) or len(_data) < 13:
    return None

tests/test_universal_remote.py covers the None response but not the short-non-empty one; a third parametrize case would mirror what tests/test_short_payload_guards.py already does on the adv-parser side.

            "charging": bool(_data[12]),
3. `\x07` dispatch branch untested and unmentioned in the PR description (carried over — still open)
switchbot/adv_parser.py:405-410

Two SUPPORTED_TYPES entries are claimed for this model, but test_universal_remote_active and test_universal_remote_charging both drive only the ' (0x27) path. Nothing parses a \x07 first byte.

Why it matters: line coverage on the added lines only proves the dict literal exists — not that \x07 actually dispatches to process_wouniversal_remote. If the byte were wrong, no test would notice.

One assertion closes it: reuse the existing test body with service_data={"0000fd3d-0000-1000-8000-00805f9b34fb": b"\x07\x00"} and assert it resolves to SwitchbotModel.UNIVERSAL_REMOTE.

Description alignment: the PR body describes only the parser and the device class. It does not say that two dispatch keys are claimed, or what \x07 represents (pairing state, going by the 0x20-offset convention the rest of the file follows). Claiming a service-data model byte is worth one line in the body so a maintainer can check it against the model registry.

    "\x07": {
        "modelName": SwitchbotModel.UNIVERSAL_REMOTE,
        "modelFriendlyName": "Universal Remote",
        "func": process_wouniversal_remote,
        "manufacturer_id": 2409,
    },

Checklist

  • Adv parser short-payload guard sized to max_index + 1
  • Dispatch keys free of collision on main
  • Dispatch key type and ordering follow file convention
  • Docstring accurate and terse per AGENTS.md (prior finding)
  • Device-level byte layout consistent with sibling devices
  • Output key naming consistent with sibling parsers — suggestion #1
  • Base-class contract preserved by override
  • Device-level index reads guarded — suggestion #2
  • Test coverage for all new dispatch branches — suggestion #3
  • Diff matches PR description (no scope creep) — suggestion #3
  • No mutable defaults / is-vs-== misuse
  • No hardcoded secrets or unsafe deserialization
  • No passive-detection manufacturer_data_length hint added
ℹ️ Triage summary

3 pre-existing finding(s) on unchanged code suppressed (freeze).


Automated review by Kōan (Claude) HEAD=4c5e133 6 min 38s

@bluetoothbot bluetoothbot 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.

Tip

No blocking issues found — ready to merge.

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.

4 participants