Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

greyeee
Copy link
Contributor

@greyeee greyeee commented Nov 25, 2024

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.

Copy link

codecov bot commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
switchbot/devices/relay_switch.py 89.47% 2 Missing ⚠️
Files with missing lines Coverage Δ
switchbot/adv_parser.py 92.85% <ø> (ø)
switchbot/adv_parsers/relay_switch.py 77.77% <100.00%> (+44.44%) ⬆️
switchbot/const.py 100.00% <100.00%> (ø)
switchbot/devices/relay_switch.py 59.66% <89.47%> (+32.21%) ⬆️

... and 1 file with indirect coverage changes


🚨 Try these New Features:

new_state,
)
if current_state != new_state:
asyncio.ensure_future(self.update())
Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Contributor Author

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?

Copy link
Member

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

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.

2 participants