-
Notifications
You must be signed in to change notification settings - Fork 27
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
Converted from the bluepy library to bleak library for bluetooth access #87
Converted from the bluepy library to bleak library for bluetooth access #87
Conversation
airthings.py converted to bleak by mjmccans
Updated version
sensor_data = sensor_decoders[str(characteristic.uuid)].decode_data(data) | ||
_LOGGER.debug("{} Got sensordata {}".format(mac, sensor_data)) | ||
|
||
|
||
# ToDo: Is there a more elegant way to handle the below? |
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.
Use an asyncio synchronization primitive instead of polling.
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.
I fully agree with the polling. I introduced an asyncio.Event. As I understand the Event it is exactly what we need here. Can you have another look?
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.
While unlikely, the challenge with event.wait() is that there is no timeout value and it could block indefinitely if something happens, like a bluetooth disconnect, at the wrong time. I suggest using wait_for instead with a 1 second timeout, which I have tested and works for me:
# Wait for up to one second to see if a callblack comes in.
try:
await asyncio.wait_for(self._event.wait(), 1)
except asyncio.TimeoutError:
_LOGGER.warn("Timeout getting command data.")
if self._command_data is not None:
sensor_data = command_decoders[str(characteristic.uuid)].decode_data(self._command_data)
self._command_data = None
# Stop notification handler
You can see the full code here.
Pin bleak to 0.14.3 Changed device scanning (updated to latest changes from mjmccans) Code cleanup
self._dev = BleakClient(mac.lower()) | ||
ret = await self._dev.connect() | ||
if ret: | ||
_LOGGER.debug("Connected to {}".format(mac)) |
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.
Retry if not connected?
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.
good finding. @mjmccans maybe you can also pull this into your repo
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.
Good catch @lymanepp. I didn't catch this error because the code still worked because if it failed to connect it tossed an exception and skipped over the break. In fact, I think the same error exists in the current code and worked for the same reason, but I agree this change makes the code more correct. Great to have more eyes on the code.
if sn is not None: | ||
if adv.addr not in self.airthing_devices: | ||
self.airthing_devices.append(adv.addr) | ||
if 820 in adv.metadata["manufacturer_data"]: # TODO: Not sure if this is the best way to identify Airthings devices |
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.
Any chance this TODO can be resolved?
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.
@lymanepp That was a note to myself, but it can probably be removed now that it has worked for others. As far as I can tell I am using the same method as before, it is just that bleak returns the values quite differently than bluepy.
Using a 1st gen Airthings Wave which doesn't support the Hardware Revision Attribute, I'm getting the following message. I think the "Error" wording might be a bit strong and may lead users to think that the component failed. Also, should we change it from a Otherwise, component works great in 2022.6.7 and 2022.7.4 !
|
@MartyTremblay I agree with your comment about line 266, and I think the following line with "self._dev = None" should be deleted as well because we should not be giving up if one of the characteristics fails to read. There may actually be an unnecessary try block as well. Perhaps the code could be cleaned up as follows:
|
airthings.py converted to bleak by mjmccans