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

Mac fixes #541

Merged
merged 4 commits into from
May 26, 2021
Merged

Mac fixes #541

merged 4 commits into from
May 26, 2021

Conversation

dlech
Copy link
Collaborator

@dlech dlech commented May 18, 2021

These are just a few small fixes I noticed while reading the code.

dlech and others added 3 commits May 15, 2021 15:47
This was commented out in f3bead7
without explanation and seems unrelated to that change.
Unfortunately, NSArray type does not support subscripting for the element type.
@@ -196,7 +197,7 @@ async def startNotify_cb_(
event = self._characteristic_notify_change_events.get_cleared(c_handle)
self.peripheral.setNotifyValue_forCharacteristic_(True, characteristic)
# wait for peripheral_didUpdateNotificationStateForCharacteristic_error_ to set event
# await event.wait()
await event.wait()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this event waiting commented out? It feels like it must have been done due to some reason once...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the commit message, it was commented out in f3bead7 and was unrelated to that change, which was about fixing RSSI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Carglglz, you remember why this was done?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dlech Sorry I missed this,

I've just review the changes I did in f3bead7 and I commented out the event wait because it would hang indefinitely when calling stopNotify, see #273. Commenting out fixed that, is there any reported issue related to this? 👀 . Also asyncio.Event seems to be not thread-safe (which is why I think it was blocking in my case), from the docs The wait() method blocks until the flag is set to true. So is there any workaround to use this with threads?

Also how does peripheral_didUpdateNotificationStateForCharacteristic_error_ to set event sets the event flag to True if event is not passed to self.peripheral.setNotifyValue_forCharacteristic_ ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bleak objects cannot be used from threads other than the one where they were created (not needing to do this is one of the benefits of using asyncio). There is call_soon_threadsafe() to schedule something on an event loop from a different thread if needed.

Could you open a new issue for the hang when stopping notifications with a reproducible code example?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also how does peripheral_didUpdateNotificationStateForCharacteristic_error_ to set event sets the event flag to True if event is not passed to self.peripheral.setNotifyValue_forCharacteristic_ ?

Ok, I remember now how it is done

def did_update_notification_for_characteristic(
        self, peripheral: CBPeripheral, characteristic: CBCharacteristic, error: NSError
    ):
        cUUID = characteristic.UUID().UUIDString()
        c_handle = characteristic.handle()
        if error is not None:
            raise BleakError(
                "Failed to update the notification status for characteristic {}: {}".format(
                    cUUID, error
                )
            )
        logger.debug("Character Notify Update")

        event = self._characteristic_notify_change_events.get(c_handle)
        if event:
            event.set()
        else:
            logger.warning(
                "Unexpected event didUpdateNotificationStateForCharacteristic"
            )

def peripheral_didUpdateNotificationStateForCharacteristic_error_(
        self, peripheral: CBPeripheral, characteristic: CBCharacteristic, error: NSError
    ):
        logger.debug("peripheral_didUpdateNotificationStateForCharacteristic_error_")
        self._event_loop.call_soon_threadsafe(
            self.did_update_notification_for_characteristic,
            peripheral,
            characteristic,
            error,
        )

I see now that self._event_loop.call_soon_threadsafe could be the reason why it was blocking. I was handling notifications in another thread with a new event loop. I will test this changes , see If it is fixed or what to do to avoid commenting out await event.wait() and works with threads too.

Copy link
Contributor

@Carglglz Carglglz May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you open a new issue for the hang when stopping notifications with a reproducible code example?

Yes, although the reproducible code example may be a little too complicated.

See https://github.com/Carglglz/bleico/tree/develop

and
https://github.com/Carglglz/bleico/blob/b7a32cef84adbbb0a1f2c7f5db9f8c90bdcc8fc4/bleico/systrayicon.py#L964

Here I'm trying to start and stop notifications in a GUI application (systray icon to be precise)... It may be a better way to do this but at that moment I didn't find any.

@dlech dlech merged commit 0758552 into hbldh:develop May 26, 2021
@dlech dlech deleted the mac-fixes branch May 26, 2021 15:39
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.

3 participants