-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix the voltage value of the relay switch #272
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
... and 1 file with indirect coverage changes 🚨 Try these New Features:
|
switchbot/devices/relay_switch.py
Outdated
new_state, | ||
) | ||
if current_state != new_state: | ||
asyncio.ensure_future(self.update()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use create_task here instead
Additionally the event loop doesn't hold strong references to tasks so we need to hold one ourselves to ensure it's not garbage collected before it finishes
We also need to handle the case where a previous update is still in progress and the task isn't finished yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your valuable advice. I have addressed the issues you pointed out. I have committed the changes and pushed them to the repository. Could you please review the updated code at your earliest convenience?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Home Assistant project relies on this project as a dependency, and we have made some necessary updates. Could you please merge the changes and release a new version at your earliest convenience?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still have a risk of multiple updates running at once
A better solution is to force poll_needed
to be True
and let Home Assistant's coordinator manage the poll in https://github.com/home-assistant/core/blob/5ef5838b20d4d6f8d340a008cb7004bb4b580228/homeassistant/components/bluetooth/active_update_coordinator.py#L168
diff --git a/switchbot/devices/relay_switch.py b/switchbot/devices/relay_switch.py
index 3c7272b..681f270 100644
--- a/switchbot/devices/relay_switch.py
+++ b/switchbot/devices/relay_switch.py
@@ -46,6 +46,7 @@ class SwitchbotRelaySwitch(SwitchbotDevice):
self._key_id = key_id
self._encryption_key = bytearray.fromhex(encryption_key)
self._model: SwitchbotModel = model
+ self._force_next_update = False
super().__init__(device, None, interface, **kwargs)
def update_from_advertisement(self, advertisement: SwitchBotAdvertisement) -> None:
@@ -68,7 +69,7 @@ class SwitchbotRelaySwitch(SwitchbotDevice):
new_state,
)
if current_state != new_state:
- asyncio.create_task(self.update())
+ self._force_next_update = True
async def update(self, interface: int | None = None) -> None:
"""Update state of device."""
@@ -90,6 +91,9 @@ class SwitchbotRelaySwitch(SwitchbotDevice):
def poll_needed(self, seconds_since_last_poll: float | None) -> bool:
"""Return if device needs polling."""
+ if self._force_next_update:
+ self._force_next_update = False
+ return True
if (
seconds_since_last_poll is not None
and seconds_since_last_poll < PASSIVE_POLL_INTERVAL
Fix the voltage value of the relay switch, combine the advertisement and obtain the voltage and current through the command to update the device state.