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

allow unpair without being connected on WinRT backend #1012

Merged

Conversation

jochenjagers
Copy link
Contributor

This is a alternative approach to #1002
Calling unpair when not connected should remove all device information from the OS.

This also relates to issue #309

Comment on lines 470 to 494
if not self._requester:
# close the device object if this was created locally
device.close()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be in a finally clause so that it still get called if an exception is raised while awaiting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is unlikely but still theoretically possible that self._requester could be assigned while awaiting, so perhaps we should just have a local should_close flag instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a try/finally block to ensure close() is called in case of an error. I also added the should_close flag.
Is it even necessary to use self._requester here at all? Can we just create a new BluetoothLEDevice for every call to unpair an simplify this function?
Something like this:

        device = await BluetoothLEDevice.from_bluetooth_address_async(
            _address_to_int(self.address)
        )

        try:
            unpairing_result = await device.device_information.pairing.unpair_async()

        finally:
            device.close()

        if unpairing_result.status not in (
            DeviceUnpairingResultStatus.UNPAIRED,
            DeviceUnpairingResultStatus.ALREADY_UNPAIRED,
        ):
            raise BleakError(f"Could not unpair with device: {unpairing_result.status}")

        logger.info("Unpaired with device.")
        return True

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not all devices have a static address. Bonded devices with privacy enabled will have a random address that changes from time to time. So from_bluetooth_address_async() should only be called as a last resort if no other information is available.

In fact, we may already have self._device_info even if self_requester is None. So we could do something like this:

if self._device_info is None:
    # BleakClient was initialized with address instead of BleDevice and has not connected yet
    device = await BluetoothLEDevice.from_bluetooth_address_async(
        _address_to_int(self.address)
    )
    device_information = device.device_information
    device.close()
else:
    device_information = self._device_info

...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

self._device_info does only contain the Bluetooth address from advertising. It is no DeviceInformation object which can be used for unpairing.
self._device_info = device.details.adv.bluetooth_address

I updated the PR and added a _create_requester function which takes the address type into account and can be used by connect and unpair to create the BluetoothLEDevice object.
For unpair the address from self._device_info is used if it is available (as it was already done in connect before). Otherwise self.address is used.
I also recognized that from_bluetooth_address_async can return None if the device is not paired and not found in the system cache. So I added a check for this case.

bleak/utils.py Outdated
)


def validate_address(address: str) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function was not used anywhere in code, so I think we can just remove it completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved it back to where it was. It would be great to might add a check to BaseBleakClient to check the address for validity to get early feedback if you pass something wrong to the client. But this is not the topic of this pull request.

bleak/utils.py Outdated
return _address_regex.match(address) is not None


def address_to_int(address: str) -> int:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this function is useful outside of the WinRT backend. If it is only needed in one file, we can just add it in that file or if it is used in more than one file, we can add a utils.py in the bleak/backends/winrt/ folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved address_to_int to WinRT's client.py.

@jochenjagers jochenjagers force-pushed the feature/winrt-unpair-without-connect-pr branch 2 times, most recently from be7cd01 to e5e68b0 Compare September 19, 2022 08:43
Copy link
Collaborator

@dlech dlech left a comment

Choose a reason for hiding this comment

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

I still think this is potentially problematic for resolvable private addresses since they change periodically, but I don't have a device to test that and it probably deserves consideration beyond the scope of this PR anyway.

finally:
device.close()
else:
logger.debug("No device found to unpair.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we raise an error instead of silently ignoring unknown devices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm really not sure about this. If you only want to remove a device from the system if it is available this is not really an error. But if you expect the device to be available for unpairing this is an error of course.
But raising a BleakError makes it hard to differentiate between an error caused by the call to WinRT backend and the device not being found. This is a problem if the "device not found" error is no problem for your use case but a "backend error" is.
If we want to raise here we should introduce a DeviceNotFound exception to be able to handle both cases in different ways.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a discussion in #527 about error types. I would say, let's just raise a BleakError for now and consider adding a DeviceNotFound error later in a wider scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a plan.

@jochenjagers jochenjagers force-pushed the feature/winrt-unpair-without-connect-pr branch from e5e68b0 to 1789ba4 Compare September 19, 2022 16:20
Copy link
Collaborator

@dlech dlech left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@dlech dlech merged commit 66f71c0 into hbldh:develop Sep 19, 2022
@jochenjagers jochenjagers deleted the feature/winrt-unpair-without-connect-pr branch September 20, 2022 07:13
pmessan pushed a commit to workaroundgmbh/bleak that referenced this pull request Sep 20, 2022
pmessan pushed a commit to workaroundgmbh/bleak that referenced this pull request Sep 20, 2022
@dlech dlech mentioned this pull request Sep 23, 2022
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