Skip to content

feat(bot): add inverse direction controls - #543

Open
bingxyz wants to merge 2 commits into
sblibs:mainfrom
bingxyz:codex/bot-inverse-direction
Open

feat(bot): add inverse direction controls#543
bingxyz wants to merge 2 commits into
sblibs:mainfrom
bingxyz:codex/bot-inverse-direction

Conversation

@bingxyz

@bingxyz bingxyz commented Jul 26, 2026

Copy link
Copy Markdown

Summary

  • expose the Bot's cached inverse-direction setting
  • add a setter that preserves the current switch mode and press strength
  • keep the existing is_on() and inverse_mode behavior unchanged

Why

SwitchBot Bot stores its reverse on/off direction setting on the device, but local BLE clients had no focused API to read and change it while preserving the other mode settings.

This was reproduced on a physical Bot connected directly to Home Assistant over Bluetooth: the device had its reverse-direction bit enabled, so on/off actions operated in the opposite physical direction. Clearing the setting through this new BLE API restored the expected behavior without the SwitchBot app, cloud access, or a device reset.

Following review, this PR is intentionally limited to reading and changing the persistent device setting. It no longer changes logical on/off state calculation or advertisement override behavior.

The Home Assistant entity that exposes this setting will be submitted separately after a PySwitchbot release containing this API is available.

Testing

  • upstream CI passed on Python 3.11–3.14
  • local full test suite on Python 3.14: 1,306 passed
  • all pre-commit hooks passed
  • re-tested the scoped implementation on a physical SwitchBot Bot through Home Assistant Container

Contribution responsibility

This implementation was AI-assisted. Responsible human: @bingxyz, who reviewed the implementation and behavior, tested it on their physical device, and authorized this submission.

Fixes #209

@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
switchbot/devices/bot.py 60.56% <100.00%> (+15.94%) ⬆️

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bdraco

bdraco commented Aug 7, 2026

Copy link
Copy Markdown
Member

@bluetoothbot review

@bluetoothbot

Copy link
Copy Markdown
Collaborator

@bdraco: @bluetoothbot review

Reviewed. Two blockers, both in is_on().

First: _is_inverse_direction() lets the device bit shadow the inverse_mode kwarg. Users with inverse_mode=True and device bit clear flip state after first update(). test_cached_inverse_direction_overrides_legacy_inverse_mode locks that in.

Second: adv_parsers/bot.py:22 hardcodes isOn: False for press mode. Inverting a sentinel yields permanent True. No switchMode=False test.

Also unverified: whether firmware pre-applies the setting before advertising. Author tested with bit cleared, so flip branch never ran on hardware.

inverse_direction() and set_inverse_direction() look correct. Worth splitting out and landing.

@bluetoothbot

Copy link
Copy Markdown
Collaborator

PR Review — feat(bot): add inverse direction controls

The new read/set API is solid; the is_on() semantics change is a silent breaking change and is not backed by the hardware testing described.

Specific strengths: set_inverse_direction() does a proper read-modify-write so it preserves switchMode and strength instead of clobbering them with set_switch_mode's defaults — that is exactly the gap in the existing API, and the parametrised test pins the wire encoding (57034b11) rather than just asserting a bool. Raising SwitchbotOperationError on an unreadable get_basic_info() matches the lock.py/meter_pro.py convention. And tests/test_bot.py is the first test module this device class has ever had — 246 lines taking switchbot/devices/bot.py from 44% to 87% coverage, following the existing generate_ble_device + AsyncMock conventions.

  • 🔴 _is_inverse_direction() lets the device-reported bit silently override the inverse_mode constructor option. Users who enabled it because their bot is mounted reversed (device bit off) get their state inverted after the first update(), with no deprecation path — and a test codifies this as intended.
  • 🟡 The inversion is applied to press-mode bots, where adv_parsers/bot.py:22 hardcodes isOn: False as a sentinel. A press-mode bot with the direction bit set reports is_on() == True permanently. No test covers switchMode=False.
  • 🟡 Unverified: whether the bot advertises the raw arm position or already applies its own inverse setting. The physical testing was done with the bit cleared, so the new flip branch never ran on hardware. If the firmware pre-applies it, this double-inverts for every affected user.
  • 🟢 isOn now means "logical" when override-sourced and "raw" when advertisement-sourced; storing the raw value in _override_state would let both the provenance branch in is_on() and the entire update_from_advertisement override be deleted.
  • 🟢 Consider splitting — inverse_direction() + set_inverse_direction() are ready to land now; the state-calculation change wants its own PR with hardware evidence.

🔴 Blocking

1. Device-reported bit silently overrides the caller's `inverse_mode` option — breaking change for existing users
switchbot/devices/bot.py:158-163

_is_inverse_direction() returns the cached inverseDirection whenever it is not None, and only falls back to self._inverse. Once update() has run and cached the device bit, the caller-supplied inverse_mode=True constructor option is silently ignored.

Why it matters — this is a public-API behaviour regression, not a refinement:

  • inverse_mode is a documented constructor kwarg on the exported Switchbot class, and Home Assistant (the primary consumer of this library, per CLAUDE.md) surfaces it as a user-facing option. The typical user who enables it does so because their bot is physically mounted reversed, i.e. the on-device inverseDirection bit is off.
  • For exactly that population — inverse_mode=True, device bit Falseis_on() returned the flipped value before this PR and returns the unflipped value after it, as soon as the first update() lands. Their switch state inverts in HA with no config change and no deprecation path.
  • test_cached_inverse_direction_overrides_legacy_inverse_mode codifies this as intended behaviour, so it will not be caught as a regression later.

Suggested fix — make the two settings compose rather than one shadow the other, or leave inverse_mode authoritative when explicitly set:

def _is_inverse_direction(self) -> bool:
    return self._inverse ^ bool(self.inverse_direction())

…or gate the device bit behind an opt-in so no existing caller changes behaviour. Either way, the semantics change deserves an explicit callout in the PR description — right now the description says "use the device-reported inverse direction when calculating logical on/off state" without mentioning that inverse_mode becomes inert.

def _is_inverse_direction(self) -> bool:
    """Return the effective inverse direction setting."""
    inverse_direction = self.inverse_direction()
    if inverse_direction is not None:
        return inverse_direction
    return self._inverse

🟡 Important

1. Inversion is applied to press-mode bots, where `isOn` is a hardcoded sentinel — yields a permanent wrong `True`
switchbot/devices/bot.py:150-151

switchbot/adv_parsers/bot.py:22 sets isOn to a hardcoded False whenever the bot is in press mode:

"isOn": not bool(data[1] & 0b01000000) if _switch_mode else False,

In press mode that False is a sentinel ("no persistent on/off state"), not a measured state. is_on() now inverts it automatically whenever the device reports inverseDirection: True, so a press-mode bot with the direction bit set will report is_on() == True permanently — it can never return False, and no command will change it.

Why it matters: press-mode + inverse-direction is a valid on-device combination (the bit lives in get_basic_info() byte 9 regardless of switchMode), and this surfaces in Home Assistant as a switch stuck in the on position. Before this PR the same flaw existed only for callers who explicitly opted into inverse_mode=True; this PR makes it automatic for anyone whose hardware has the bit set.

Suggested fix — only invert when the bot actually has a persistent state:

if self.switch_mode() and self._is_inverse_direction():
    return not value

The new test suite does not cover this: every make_advertisement_data payload hardcodes switchMode: True. A switchMode=False + inverseDirection=True case would pin the behaviour down.

if self._is_inverse_direction():
    return not value
return value
2. The core premise — that the advertised `isOn` needs flipping by the device bit — is not verified by the stated hardware testing
switchbot/devices/bot.py:148-152

The whole is_on() change rests on one unproven assumption: that the bot advertises the raw arm position and leaves the client to apply the on-device inverse-direction setting, rather than applying it itself before advertising.

I could not confirm this either way from the codebase — adv_parsers/bot.py just decodes bit 6 of data[1] with no documentation of whether the firmware pre-applies the setting, and there is no prior test or comment establishing it. Flagging as unverified.

Why it matters — the two outcomes are opposite:

  • If the firmware does not pre-apply it, this change is a genuine fix.
  • If it does, is_on() now double-inverts for every bot user with the direction bit set, reporting the exact opposite state. That is a silent, widespread wrong-state bug in a library that backs the Home Assistant integration.

The PR's hardware testing does not settle it. The description says the physical bot had the bit enabled and that clearing it "restored the expected behavior" — that validates set_inverse_direction(), but with the bit cleared _is_inverse_direction() returns False and the new flip path is a no-op. The branch that actually matters was never exercised on real hardware.

Suggested path: on the physical bot, set inverseDirection: True, then compare the raw advertised isOn against the physical arm position and against what the SwitchBot app displays. Paste that observation into the PR. Until then this branch should not ship.

if self._override_adv_data and "isOn" in self._override_adv_data:
    return value
if self._is_inverse_direction():
    return not value
return value

🟢 Suggestions

1. `isOn` now carries two different meanings depending on provenance; storing the raw value in the override removes both workarounds
switchbot/devices/bot.py:37-49

turn_on()/turn_off() call _override_state({"isOn": True/False}) with the logical value, while advertisements supply the raw value — and both land under the same isOn key in _override_adv_data and (via _override_state_update_parsed_data) in parsed_data.

That ambiguity is what forces the two new workarounds:

  • the provenance branch at the top of is_on() (if self._override_adv_data and "isOn" in ...: return value) — needed only to skip inversion for override-sourced values;
  • this whole update_from_advertisement override — needed only because the logical value left behind in parsed_data would otherwise be re-inverted once the override is cleared by an isOn-less advertisement.

It also leaks: parsed_data["isOn"] / _get_adv_value("isOn") mean "logical" after a command and "raw" after an advertisement. test_turn_on_off_optimistic_state_respects_inverse_direction asserts the logical reading, so any other consumer of that key is now reading an inconsistent field.

A simpler shape: store the raw value in the override, matching what advertisements carry —

async def turn_on(self) -> bool:
    ...
    self._override_state({"isOn": not self._is_inverse_direction()})

— after which is_on() is just not value if self._is_inverse_direction() else value with no provenance check, and update_from_advertisement can be dropped entirely (the stale raw value in parsed_data is already correct to invert). Fewer moving parts and one consistent meaning for the key.

def update_from_advertisement(self, advertisement: SwitchBotAdvertisement) -> None:
    """Update data without losing an optimistic state to an incomplete payload."""
    had_is_on_override = bool(
        self._override_adv_data and "isOn" in self._override_adv_data
    )

Checklist

  • No backward-incompatible changes to public APIs — critical #1, warning #1
  • Logic correct for edge cases (press mode, missing isOn, None state) — warning #1, warning #2
  • Behaviour claims verified against hardware or codebase evidence — warning #2
  • Untested branches introduced by the change — warning #1
  • Abstraction level and key semantics are coherent — suggestion #1
  • New public API has real callers / clear purpose (YAGNI)
  • No hardcoded secrets or unsafe operations
  • Error handling — no swallowed exceptions, no leaked internals
  • Test conventions match the existing suite (pytest-asyncio, generate_ble_device)
  • Tests verify observable behaviour, not source inspection
  • Code style matches repo conventions (single-line docstrings, py311, no rationale comments)
  • Autonomous contribution attributed to a responsible human

To rebase and address feedback, mention me: @bluetoothbot rebase critical (fixes 🔴 only), @bluetoothbot rebase important (fixes 🔴 + 🟡), or @bluetoothbot rebase --fix for all. (A bare @bluetoothbot rebase only rebases onto the base branch.)


Automated review by Kōan (Claude) HEAD=ae943e4 5 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.

Caution

Critical issues found.

  • Device-reported bit silently overrides the caller's inverse_mode option — breaking change for existing users
  • Inversion is applied to press-mode bots, where isOn is a hardcoded sentinel — yields a permanent wrong True
  • The core premise — that the advertised isOn needs flipping by the device bit — is not verified by the stated hardware testing

@bingxyz

bingxyz commented Aug 10, 2026

Copy link
Copy Markdown
Author

Addressed the review in 9232db8:

  • removed device-reported inverse direction from is_on() state calculation
  • removed the Bot-specific update_from_advertisement() override and related optimistic-state handling
  • preserved the existing inverse_mode behavior
  • scoped the PR to inverse_direction() and set_inverse_direction()
  • re-ran the full suite and pre-commit checks, and re-tested through Home Assistant on the physical Bot

The updated CI is green. @bluetoothbot review

@bluetoothbot

Copy link
Copy Markdown
Collaborator

❌ Permission denied. Only users with write access can trigger bot commands.

@bingxyz

bingxyz commented Aug 10, 2026

Copy link
Copy Markdown
Author

Hi @bdraco, I addressed the bot review by removing the is_on() behavior changes and limiting the PR to the read/set API as suggested. CI is green and the updated implementation was tested on the physical Bot. Could you please retrigger @bluetoothbot review when convenient? Thanks!

@bluetoothbot

Copy link
Copy Markdown
Collaborator

❌ Permission denied. Only users with write access can trigger bot commands.

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.

[bot] inversion ON/OFF

3 participants