feat: add roller mode for switch2pm by fankai777 · Pull Request #457 · sblibs/pySwitchbot

fankai777 · 2026-03-05T09:36:59Z

Summary

Add roller/cover mode support for SwitchbotRelaySwitch2PM and fix related test cases.

Changes

Features

SwitchbotRelaySwitch2PM now inherits from SwitchbotBaseCover, enabling cover/roller control support.
Added open(), close(), stop(), set_position() commands for roller mode.
Added position and mode properties to SwitchbotRelaySwitch2PM.
Added roller mode BLE commands: COMMAND_OPEN, COMMAND_CLOSE, COMMAND_POSITION, COMMAND_STOP.

Parser

process_relay_switch_2pm: Added mode, position, calibration fields to both channel 1 and channel 2 advertisement data.

Refactor

SwitchbotBaseCover.__init__: Changed reverse to a keyword-only argument, updated all subclass call sites (curtain.py, blind_tilt.py, roller_shade.py).
Fixed _parse_common_data in SwitchbotRelaySwitch: sequence_number now reads from raw_data[0] (was raw_data[1]).

Tests

Updated test_adv_parser.py: Added expected mode, position, calibration fields to SwitchbotRelaySwitch2PM test cases.
Updated test_base_cover.py: Fixed SwitchbotBaseCover instantiation to use new keyword argument signature.

codecov · 2026-03-05T09:44:05Z

Codecov Report

❌ Patch coverage is 95.77465% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
switchbot/devices/relay_switch.py	95.16%	3 Missing ⚠️

Files with missing lines	Coverage Δ
switchbot/adv_parsers/relay_switch.py	`100.00% <100.00%> (ø)`
switchbot/devices/base_cover.py	`100.00% <100.00%> (ø)`
switchbot/devices/blind_tilt.py	`98.64% <100.00%> (ø)`
switchbot/devices/curtain.py	`98.66% <100.00%> (-0.02%)`	⬇️
switchbot/devices/roller_shade.py	`100.00% <100.00%> (ø)`
switchbot/devices/relay_switch.py	`98.45% <95.16%> (-1.55%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This PR adds roller/cover mode support for SwitchbotRelaySwitch2PM by having it additionally inherit from SwitchbotBaseCover. It also refactors the SwitchbotBaseCover constructor to accept reverse as a keyword argument, updates the advertisement parser for 2PM to include mode, position, and calibration fields, and updates tests accordingly.

Changes:

SwitchbotRelaySwitch2PM now inherits from both SwitchbotRelaySwitch and SwitchbotBaseCover, gaining cover/roller commands (open, close, stop, set_position) and properties (position, mode).
SwitchbotBaseCover.__init__ changed reverse from a positional to a keyword argument; all subclass call sites updated.
Advertisement parser and _parse_common_data extended with mode, position, calibration fields for 2PM devices.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`switchbot/devices/base_cover.py`	Refactored constructor to accept `reverse` as keyword-only via `kwargs.pop`
`switchbot/devices/relay_switch.py`	Added cover inheritance, roller commands, position/mode properties, motion direction logic, and refactored 2PM `get_basic_info`
`switchbot/adv_parsers/relay_switch.py`	Added `mode`, `position`, `calibration` fields to 2PM advertisement parser
`switchbot/devices/curtain.py`	Updated `super().__init__` call for new keyword arg signature
`switchbot/devices/blind_tilt.py`	Updated `super().__init__` call for new keyword arg signature
`switchbot/devices/roller_shade.py`	Updated `super().__init__` call for new keyword arg signature
`tests/test_relay_switch.py`	Added tests for cover operations, position, mode, and motion direction
`tests/test_base_cover.py`	Updated `SwitchbotBaseCover` instantiation to match new keyword arg signature
`tests/test_adv_parser.py`	Added expected `mode`, `position`, `calibration` fields to 2PM test cases

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

bdraco · 2026-04-23T11:42:01Z

Out of time to finish reviewing this toda. Will try to pickup reviews on this repo later this week.

PR sblibs#457 changed `SwitchbotBaseCover.__init__` to read `reverse` from `**kwargs` so it can travel through the MRO when `SwitchbotRelaySwitch2PM` inherits from both `SwitchbotRelaySwitch` and `SwitchbotBaseCover`. That silently broke any external caller passing `reverse` as a leading positional arg — `reverse` would be ignored and the bool would be forwarded to `super().__init__` as `device`. Restore backward compatibility by accepting either form: a leading positional bool (legacy) or a `reverse=` kwarg (preferred). The bool sniff is unambiguous because the only other candidate for the first positional is `BLEDevice`. Adds tests covering the legacy positional form, the kwarg form, and the omitted-default form.

bdraco · 2026-04-23T21:01:18Z

+        return success
+
+    @update_after_operation
+    async def set_position(self, position: int, mode: int = 0) -> bool:


This doesn't appear to honor reverse

Now honored. set_position applies 100 - position when reversed.

bdraco · 2026-04-23T21:03:29Z

+        return success
+
+    @update_after_operation
+    async def set_position(self, position: int, mode: int = 0) -> bool:


mode is unused?

Resolved. mode parameter removed from set_position.

bdraco

Please see review comments above

Onero-testdev · 2026-06-17T07:38:06Z

Thanks for the detailed review! All issues have been addressed in the latest commit (c62e071):

Copilot comments:

Operator precedence bug — Fixed: (mfr_data[9] & 0b11110000) >> 4 now has correct parentheses.
Abstract methods not implemented — Fixed: get_position() and get_extended_info_summary() are now explicitly implemented in SwitchbotRelaySwitch2PM.

@bdraco comments:

Code duplication — Cleaned up: open(), close(), stop(), and set_position() have been refactored to remove duplication.
set_position doesn't honor reverse — Fixed: set_position() now applies position = 100 - position when self._reverse is True.
open/close don't honor reverse — Fixed: open() sends device position 100 when reversed, close() sends 0, matching the base cover convention.
mode parameter unused — Fixed: the mode parameter has been removed. The protocol's 0x0D 0x04 command only takes position + control-source bytes; there is no mode field.
Same position byte for channel 1 and 2 — Confirmed correct per the BLE protocol spec. In roller/cover mode, both relay channels drive a single motor together — there is only one position byte in the advertisement (Byte 21, mfr_data[14]). Both channels share the same physical position, so reading from the same byte is intentional.

Also added a regression test (test_2pm_adv_short_mfr_data_returns_empty) to guard against truncated advertisement payloads. All 1101 tests pass.

bluetoothbot · 2026-06-20T13:47:56Z

~~Previous review~~ — superseded by a newer review below.

bluetoothbot

Blocking issues found.

COMMAND_OPEN / COMMAND_CLOSE are dead code
Two different encodings for "position" (basic-info vs advertisement)

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

switchbot/devices/relay_switch.py:214

SwitchbotRelaySwitch2PM now inherits SwitchbotBaseCover, but unlike the other cover devices it does not consume reverse_mode (or reverse) from **kwargs. Because SwitchbotBaseCover.__init__ only pops reverse, passing reverse_mode= to the constructor will be silently ignored, making it impossible to flip direction consistently with SwitchbotCurtain/SwitchbotRollerShade/SwitchbotBlindTilt.

class SwitchbotRelaySwitch2PM(SwitchbotRelaySwitch, SwitchbotBaseCover):
    """Representation of a Switchbot relay switch 2pm."""

    _model = SwitchbotModel.RELAY_SWITCH_2PM
    _channel: int = 2

 def process_relay_switch_2pm(
    data: bytes | None, mfr_data: bytes | None
 ) -> dict[int, dict[str, Any]]:
    """Process Relay Switch 2PM services data."""
-    if mfr_data is None:
+    # Highest index read below is mfr_data[14] (roller position), so guard
+    # against truncated advertisements that would otherwise raise IndexError.
+    if mfr_data is None or len(mfr_data) < 15:
        return {}


    def _parse_common_data(self, raw_data: bytes) -> dict[str, Any]:
        """Parse common data from raw bytes."""
        return {
            "sequence_number": raw_data[1],
            "isOn": bool(raw_data[2] & SWITCH1_ON_MASK),
            "firmware": raw_data[16] / 10.0,


Onero-testdev · 2026-06-22T09:12:05Z

All review feedback addressed — summary of changes in this push:

Removed dead code (COMMAND_OPEN / COMMAND_CLOSE): open() and close() both send COMMAND_POSITION, so the two named constants were unused. Deleted them and updated the corresponding test assertions to match the actual command sent.
Fixed return type annotation on process_relay_switch_2pm: was dict[int, dict[str, Any]]; corrected to dict[int | str, dict[str, Any] | int] to reflect the top-level "sequence_number" int value alongside the per-channel dicts.
Extracted _send_position() helper: open(), close(), and set_position() all shared the same _send_command(COMMAND_POSITION.format(...)) + _check_command_result(...) pattern. Extracted into a private async def _send_position(self, position: int) -> bool method; motion-direction logic preserved in the callers.
Merged upstream/master: clean merge, no conflicts (35 files updated).

Tests: 55 relay-switch tests pass, 1238 total pass. ruff check + format clean.

- Add open/close/set_position commands for 2PM roller/cover mode - Add _send_position() helper to deduplicate position command logic - Fix type annotation in adv_parsers/relay_switch.py - Clean up dead code (COMMAND_OPEN/COMMAND_CLOSE) - Add comprehensive tests for roller mode and short-response guards

Onero-testdev · 2026-06-22T10:59:56Z

All review feedback has been addressed:

Removed dead code (COMMAND_OPEN / COMMAND_CLOSE)
Fixed the return type annotation in adv_parsers/relay_switch.py
Extracted _send_position() helper to eliminate duplication in open/close/set_position
Resolved merge conflicts with upstream/main — all CI checks now passing

Could you please take another look? Thanks!

bluetoothbot · 2026-06-22T13:21:57Z

~~Previous review~~ — superseded by a newer review below.

bluetoothbot

Blocking issues found.

Two different position encodings reach Home Assistant for the same logical value

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Onero-testdev · 2026-06-24T09:07:20Z

Fixed the position encoding inconsistency flagged by bluetoothbot.

Root cause: _parse_common_data (used by get_basic_info) was applying >> 1 to the position byte, producing 0–50 instead of the correct 0–100 range. The advertisement parser (process_relay_switch_2pm) correctly uses the raw mfr_data[14] value. These two different encodings reaching Home Assistant caused the "Two different position encodings" issue.

Fix: Removed the >> 1 shift — raw_data[9] is now used directly, matching the advertisement's encoding. Per the protocol documentation, the position is always a direct 0–100 percentage value (broadcast byte 21, 0x0F 0x71 0x0d response, and 0x0F 0x71 0x81 response).

Changes:

switchbot/devices/relay_switch.py: "position": raw_data[9] >> 1 → "position": raw_data[9]
Added regression test: raw_data[9] = 0x64 (100) → asserts position == 100

All CI checks passing.

bluetoothbot · 2026-06-24T09:14:43Z

@Onero-testdev: Fixed the position encoding inconsistency flagged by bluetoothbot.

Range aligned, source still unconfirmed. Advertisement reads mfr_data[14]; basic_info reads raw_data[9]. Different transports, different bytes. Removing >>1 fixes scale, not orientation or source parity. Confirm both report identical 0-100 via BLE capture. Otherwise position jumps between advert and poll.

bluetoothbot · 2026-06-24T09:14:46Z

PR Review — feat: add roller mode for switch2pm

Solid mechanics; the remaining risk is position semantics, which only a hardware capture can settle.

What's done well:

The BaseCover.__init__ refactor to *args/**kwargs is genuinely necessary, not cosmetic — it's what lets reverse travel through the MRO so SwitchbotRelaySwitch2PM(SwitchbotRelaySwitch, SwitchbotBaseCover) constructs without EncryptedDevice clobbering the reverse slot. Verified the C3 linearization and the existing 2PM test instantiation both hold.
All three BaseCover abstract methods are implemented, so the earlier TypeError risk is gone.
Earlier-flagged issues are actually resolved in this diff: operator-precedence on the channel-2 mode nibble, the COMMAND_OPEN/COMMAND_CLOSE dead constants, the _send_position extraction, and the short-payload guard (len(mfr_data) < 15) sized to the max index read.

What needs attention:

Position scale across the two transports (adv mfr_data[14] vs basic_info raw_data[9]) is unverified — risks HA position jumping between passive and active updates.
Position orientation: 2PM hardcodes _reverse=False and exposes the raw device byte, inverse to the HA 0=closed convention every sibling cover normalizes to; the reverse branches are also dead code.
_update_motion_direction is duplicated verbatim from roller_shade.
get_position() duplicates the position property.
Cover-only fields emitted for switch-mode 2PM units.

🟡 Important

1. Position from advertisement (mfr_data[14]) and basic_info (raw_data[9]) are unverified to share the same scale
switchbot/adv_parsers/relay_switch.py:57-63

position is sourced from two independent byte streams that must agree, but nothing here proves they do:

Advertisement parser: "position": mfr_data[14] (raw BLE manufacturer-data byte).
_parse_common_data (used by get_basic_info): "position": raw_data[9] (GATT basic-info response byte).

These are different transports. The author's last update removed the >> 1 shift to align the range, but that does not prove the two source bytes both encode a direct 0–100 percentage on the same orientation.

Why it matters: get_position()/position read the cached advertisement value, while a polled get_basic_info() overwrites the cache with raw_data[9]. If the two encodings differ (e.g. one is 0–100, the other 0–255 or value*2), Home Assistant will see the roller position jump every time it switches between passive advertisement updates and an active poll.

This can only be settled by a BLE capture of a 2PM in roller mode comparing mfr_data[14] against the 0x0F 0x71 0x06 channel-1 response byte 9. Please confirm both before merge, or note in the PR that it was hardware-verified.

"position": mfr_data[14],   # advertisement
"position": raw_data[9],     # basic_info

2. 2PM position orientation is inverted relative to sibling covers / HA convention
switchbot/devices/relay_switch.py:213-262

Every other cover sets reverse_mode to deliver Home Assistant's 0 = closed, 100 = open convention: curtain/roller_shade default reverse_mode=True, blind_tilt defaults False because its raw orientation already matches (see the explanatory comments in curtain.py:41-47, roller_shade.py:36-37, blind_tilt.py:40-43).

SwitchbotRelaySwitch2PM never pops a reverse_mode kwarg, so BaseCover.__init__ leaves self._reverse = False permanently. Combined with open() sending device position 0 and close() sending 100, the exposed position/get_position() returns the raw device byte where 0 = open — the inverse of the HA convention the sibling covers go out of their way to normalize.

Why it matters: unless the 2PM advertisement byte happens to be pre-inverted in hardware, HA will render a fully-open roller as closed (and vice-versa). The 100 - x conversions inside set_position's _update_motion_direction call hint the author knows device-space is inverted from HA-space — but that same inversion is never applied to the value returned by the getter.

Either expose reverse_mode like the other covers (and apply it to the getter), or confirm via capture that mfr_data[14]'s orientation already matches HA. As-is the reverse branches in open/close/set_position are also dead code since _reverse can never be True.

position = 100 if self._reverse else 0   # open(); _reverse is always False

🟢 Suggestions

1. _update_motion_direction is copy-pasted verbatim from roller_shade.py
switchbot/devices/relay_switch.py:372-385

This method is byte-for-byte identical to SwitchbotRollerShade._update_motion_direction (roller_shade.py:155-167). Two copies of the same motion-state logic will drift over time.

Consider hoisting it onto SwitchbotBaseCover so both covers share one implementation.

Note one behavioral difference worth a second look: roller_shade passes raw positions to this method, while 2PM's set_position passes 100 - x (inverted). They can't both be right about which direction is "opening" — worth confirming the 2PM orientation against the same capture as the warnings above.

2. get_position() duplicates the position property
switchbot/devices/relay_switch.py:234-246

get_position() and the position property both return self._get_adv_value("position", channel=1) verbatim. get_position() exists only to satisfy BaseCover's @abstractmethod, so having both is understandable, but a future edit to one can silently diverge.

Have the abstract method delegate to the property for a single source of truth:

def get_position(self) -> Any:
    return self.position

3. mode/position/calibration emitted for every 2PM, including plain dual-switch units
switchbot/adv_parsers/relay_switch.py:53-65

mode, position, and calibration are added unconditionally to both channel dicts of every RELAY_SWITCH_2PM advertisement. A 2PM wired as a plain dual switch (not a roller) still reports a position derived from mfr_data[14], which is meaningless in that configuration.

Not harmful — the fields are inert extra keys — but a downstream consumer can't distinguish a real roller position from a placeholder. Since the channel-1 mode nibble (mfr_data[9] & 0b00001111) already distinguishes roller from switch operation, consider gating the cover-only fields on that value.

Checklist

Multiple-inheritance MRO resolves; no abstract-method TypeError
reverse honored in open/close/set_position — warning ＃2
Consistent position encoding across adv and basic_info paths — warning ＃1
Short-payload parser guard sized to max index
No dead code / unused constants — warning ＃2
No duplicated logic — suggestion ＃1, suggestion ＃2
Fields scoped to applicable device modes — suggestion ＃3
No mutable default args / is-vs-== misuse

To rebase specific severity levels, mention me: @bluetoothbot rebase critical (fixes 🔴 only), @bluetoothbot rebase important (fixes 🔴 + 🟡), or just @bluetoothbot rebase for all.

Automated review by Kōan (Claude) HEAD=330ddd3 5 min 30s

bluetoothbot

Blocking issues found.

Position from advertisement (mfr_data[14]) and basic_info (raw_data[9]) are unverified to share the same scale
2PM position orientation is inverted relative to sibling covers / HA convention

bdraco requested review from bdraco and Copilot March 13, 2026 19:23

Copilot started reviewing on behalf of bdraco March 13, 2026 19:24 View session

Copilot AI reviewed Mar 13, 2026

View reviewed changes

Comment thread switchbot/adv_parsers/relay_switch.py Outdated

Comment thread switchbot/devices/relay_switch.py

bdraco reviewed Apr 23, 2026

View reviewed changes

Comment thread switchbot/devices/relay_switch.py

bdraco reviewed Apr 23, 2026

View reviewed changes

Comment thread switchbot/devices/relay_switch.py

bdraco reviewed Apr 23, 2026

View reviewed changes

Comment thread switchbot/adv_parsers/relay_switch.py

bdraco requested changes Apr 24, 2026

View reviewed changes

bluetoothbot requested changes Jun 20, 2026

View reviewed changes

bdraco requested a review from Copilot June 20, 2026 14:08

Copilot started reviewing on behalf of bdraco June 20, 2026 14:09 View session

Copilot AI reviewed Jun 20, 2026

View reviewed changes

fankai777 changed the title ~~add roller mode for switch2pm~~ feat: add roller mode for switch2pm Jun 22, 2026

Onero-testdev force-pushed the feat/2pm branch from ea5131c to 49be3a3 Compare June 22, 2026 10:36

bluetoothbot requested changes Jun 22, 2026

View reviewed changes

fix: align position encoding between advertisement and get_basic_info

330ddd3

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

bluetoothbot requested changes Jun 24, 2026

View reviewed changes

Uh oh!

Conversation

fankai777 commented Mar 5, 2026

Summary

Changes

Features

Parser

Refactor

Tests

Uh oh!

codecov Bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bdraco commented Apr 23, 2026

Uh oh!

bdraco Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

bluetoothbot Jun 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bdraco Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

bluetoothbot Jun 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bdraco left a comment

Choose a reason for hiding this comment

Uh oh!

Onero-testdev commented Jun 17, 2026

Uh oh!

bluetoothbot commented Jun 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluetoothbot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Onero-testdev commented Jun 22, 2026

Uh oh!

Onero-testdev commented Jun 22, 2026

Uh oh!

bluetoothbot commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluetoothbot left a comment

Choose a reason for hiding this comment

Uh oh!

Onero-testdev commented Jun 24, 2026

Uh oh!

bluetoothbot commented Jun 24, 2026

Uh oh!

bluetoothbot commented Jun 24, 2026

PR Review — feat: add roller mode for switch2pm

🟡 Important

🟢 Suggestions

Checklist

Uh oh!

bluetoothbot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

codecov Bot commented Mar 5, 2026 •

edited

Loading

bluetoothbot commented Jun 20, 2026 •

edited

Loading

bluetoothbot commented Jun 22, 2026 •

edited

Loading