Specify scan mode as boolean instead of string literal

[bojanpotocnik]

Split from #1140

[dlech]

What is the motivation for these changes?

For me, a more compelling example would be something along the lines of...

# pseudocode

scan_mode = "passive"

if platform_does_not_support_passive_scanning():
    scan_mode = "active"
    print("WARNING! Passive scanning is not supported on this platform.")
    print("Don't leave this running for a long time, otherwise it will drain all of your batteries!")

async with BleakScanner(scan_mode):
    wait_forever()

Treating active scanning as a fallback for passive scanning doesn't seem like something that should be encouraged.

[bojanpotocnik]

I see your point - buried in my use-case(s), I did not think of this library being used as running scans for a long time on a battery powered devices.

• I added enum instead of string literal because, well, they're made for such use cases, and I'm influenced by the C/++ background. It's true that we have a typing.Literal, but it is still more convenient when using code completion.
• I added example because I spent some time searching for proper or_patterns and thought such example might be helpful.
• I added additional exception to detect this error in more correct way than catching BleakError and checking if "passive" in str(e): # try active

I like your idea of using separate function, or maybe BleakScanner static method to check for passive scanning support. However, I am going through the BlueZ source code for 2 days now and I'm afraid we cannot check that for BlueZ prior to starting the scanning. SupportedMonitorTypes and SupportedFeatures of AdvertisementMonitorManager do not perform any MGMT commands and their result depends solely on the BlueZ version, irrespective of the Linux kernel version.

If such check would be possible, then TRY_PASSIVE could for sure be removed from the enum. After that, the enum holds only 2 options... In that case I'm wondering why not just use passive_scan: bool as a parameter?

[dlech]

In that case I'm wondering why not just use passive_scan: bool as a parameter?

The answer is the same reason we didn't use an enum. It was already added to Bleak long ago as a string and we have just left it that way because it is working even if not ideal.

However, I am going through the BlueZ source code for 2 days now and I'm afraid we cannot check that for BlueZ prior to starting the scanning.

Just brainstorming here... What if we do both? The platform support check function for BlueZ could return True that BlueZ, in general, has support for passive scanning. But at runtime, raise exceptions for the cases where it won't work as we currently do (BlueZ too old, experimental features not enabled, kernel too old).

For users that need more robust checking for features at runtime but before actually scanning, we could add some aysnc helper functions specifically for BlueZ using the existing BlueZFeatures class. (As a side note, to support sandboxed apps as in #798, I would like to eventually git rid of the other BlueZ version checks we are currently using this class for - we could keep the checks in the BlueZFeatures class, but not use them in other Bleak code).

Instead of the kernel version check, we could call a subprocess with btmgmt revision to test if the kernel supports a feature such as advertisement monitors.

[bojanpotocnik]

I thought of implementing the check function as BleakScanner member, as it seemed the most fitting:

class BleakScanner:
    ...

    @property
    def supports_passive_scanning(self) -> bool:
        return self._backend.supports_passive_scanning

    # Or
    async def supports_passive_scanning(self) -> bool:
        """Check whether passive scanning is possible on this platform"""
        return await self._backend.supports_passive_scanning()

class BaseBleakScanner(abc.ABC):
    ...

    # @staticmethod
    @abc.abstractmethod
    async def supports_passive_scanning(self) -> bool:
        """Check whether passive scanning is possible on this platform"""
        raise NotImplementedError()

which would work well, because if/when bluez/bluez#434 is fixed, something like this could be used to check if passive scanning is supported on the system:

class BleakScannerBlueZDBus(BaseBleakScanner):
    ...

    async def supports_passive_scanning(self) -> bool:
        reply = await self._bus.call(
            Message(
                destination=defs.BLUEZ_SERVICE,
                path=self._adv_monitor_path,
                interface=defs.PROPERTIES_INTERFACE,
                member="Get",
                signature="ss",
                body=[defs.ADVERTISEMENT_MONITOR_MANAGER_INTERFACE, "SupportedFeatures"],
            )
        )
        return reply.message_type != MessageType.ERROR

However, downside of such approach is that BleakScanner._backend is chosen only after BleakScanner is instantiated, so one cannot use it in the context manager as

async with BleakScanner(
    scan_callback,
    scanning_mode="passive" if await BleakScanner.supports_passive_scanning() else "active"
):
    ...

Here something like "try passive" would come handy... or we make a truly separate function, similar to get_platform_scanner_backend_type() - but I'm not sure why I don't like this "abstract function" approach (and another D-Bus connection needs to be made).

[bojanpotocnik]

I've started implementing it in a way

class BleakScanner:
    @staticmethod
    async def supports_passive_scanning(backend: Optional[Type[BaseBleakScanner]] = None) -> bool:
        """Check if passive scanning mode is possible in this platform"""
        return await (get_platform_scanner_backend_type() if backend is None else backend).supports_passive_scanning()

class BaseBleakScanner(abc.ABC):
    @staticmethod
    @abc.abstractmethod
    async def supports_passive_scanning() -> bool:
        """Check if passive scanning mode is possible in this platform"""
        raise NotImplementedError()

class BlueZManager:
    async def supports_passive_scanning(self, adapter_path: str) -> bool:
        reply = await self._bus.call(
            Message(
                destination=defs.BLUEZ_SERVICE,
                path=adapter_path,
                interface=defs.PROPERTIES_INTERFACE,
                member="Get",
                signature="ss",
                body=[
                    defs.ADVERTISEMENT_MONITOR_MANAGER_INTERFACE,
                    "SupportedFeatures",
                ],
            )
        )
        return reply.message_type != MessageType.ERROR

class BleakScannerBlueZDBus(BaseBleakScanner):
    @staticmethod
    async def supports_passive_scanning() -> bool:
        manager = await get_global_bluez_manager()

        return await manager.supports_passive_scanning(manager.get_default_adapter())

but what about

        if self._adapter:
            adapter_path = f"/org/bluez/{self._adapter}"
        else:
            adapter_path = manager.get_default_adapter()

in BleakScannerBlueZDBus.supports_passive_scanning()?
No matter how I try to turn it around, I come back to requiring BleakScannerBlueZDBus instantiated with the actual arguments which will later be used for scanning. So I would need to do something like

scanner = BleakScanner(scan_callback, adapter=adapter)
if scanner.supports_passive_scanning():
    scanner.scanning_mode = "passive"  # This property does not exist
async with scanner:
    ...

Ok, let's say we are OK with this. However, because in case of BlueZ, missing kernel support can only be detected after scanning is started

async with scanner:
    ...

we still need to try/except BleakNoPassiveScanError at this point (#1140), and this cannot be removed easily to support different (older) kernel and BlueZ versions (some of the future BlueZ will hopefully have this fixed - bluez/bluez#434).

So (for now), if try/except must be there and handled anyway, it might just be the sole way of detecting the unsupported passive scanning.

[bojanpotocnik]

One thing observed when testing example added in #1140, considering the usage of string literals in typing.

async with BleakScanner(scan_callback, scanning_mode="passive" if passive_mode_supported else "active"):
    await asyncio.sleep(60)

causes PEP Check warning

Expected type 'Literal["active", "passive"]', got 'str' instead

which does not happen with enums.

[bojanpotocnik]

This is now kept open just to avoid warning in the previous comment. If you think it is not worth the change, it can as well be closed.

[dlech]

I would be OK with adding an enum and changing the type hint for scanning_mode to that enum type as long as the string values still work for backwards compatibility.

Closing this PR since changing the name of the parameter and changing the type to bool is a breaking change.