bluez: Verify that Advertisement Monitor has been registered

[bojanpotocnik]

Solves #1136

[bojanpotocnik]

Another option would be to execute

                reply = await self._bus.call(
                    Message(
                        destination=defs.BLUEZ_SERVICE,
                        path=monitor_path,
                        interface=defs.PROPERTIES_INTERFACE,
                        member="Get",
                        signature="ss",
                        body=[defs.ADVERTISEMENT_MONITOR_INTERFACE, "Type"],
                    )
                )

however this fails with ['Method "Get" with signature "ss" on interface "org.freedesktop.DBus.Properties" doesn\'t exist\n']. Considering that even await asyncio.sleep(0.1) was sufficient here, I left this call which always succeeds.

[dlech]

I think asyncio.sleep(0) is idiomatic if we just need to yield from the coroutine momentarily.

[bojanpotocnik]

Tested, and it is not enough. I think that await asyncio.sleep(0) does yield, but it does not assure that the D-Bus event/operation queue is completely processed, or processed at all. Just for curiosity, asyncio.sleep(0.001) "always" does the job, 0.0001 does the job in about half of the tries, 0.00001 is never enough.

[dlech]

This is an interesting idea. Since Release being called could be for other reasons besides this one particular case, maybe it is better to just stick with the os.uname() check and fail early?

[dlech]

I think asyncio.sleep(0) is idiomatic if we just need to yield from the coroutine momentarily.

[bojanpotocnik]

Since Release being called could be for other reasons besides this one particular case ...

My first thought - before further analysing the BlueZ source code - was that even though the Release is called for other reasons, it shall for sure not be called before this function returns.

Checking the source https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/adv_monitor.c#n520, I see that D-Bus Release method is invoked only from monitor_release(), which is called only from monitor_destroy(), which is called from 5 functions, all of them related to removal of the monitor as a result of deliberate action, error, or invalid parameters.

... maybe it is better to just stick with the os.uname() check and fail early?

I like the os.uname() idea, but would prefer to solve this as BlueZ-ish as possible - maybe someone can use a custom kernel or kernel patched to have or have not support for this feature... Most probably not, but still - this was a fun/frustrating challenge for today.

[bojanpotocnik]

Done some more testing, using kernel <5.10 and >5.10.

In case of kernel <5.10, Release() is invoked during the call of SupportedFeatures(), as expected.

In case of kernel >5.10, Activate() is called some time after this function already returns - e.g. if using:

async with BleakScanner(..., scanning_mode="passive"):
    await asyncio.sleep(0.0001)

no AdvertisementMonitor method is invoked, but when this delay is increased, Activate() is invoked. Interestingly, Release() is not invoked at all, even if I add some sleep after the context manager.

I added example file: On one hand, I like to show how one can use async iterator to print detected devices, but on the other hand, it gets complicated. I can remove or simplify it if you prefer, I'll do PR for BleakScanner supporting async iteration anyway. it tries to use the passive scanning first and falls back to active scanning if not possible.

[dlech]

The scope of this PR is creeping. Can we please stick to just one change at a time?

Regarding checking if Release has been called or not, I've thought about it a bit more and it seems like we should make this cancel any async task in the async context manager and raise an exception similar to how asyncio.timeout works. This would avoid any race condition of when Release is called and we wouldn't have to rely on the hack of getting a property to get the right timing and would also cover any other reason for Release being called at any time.

but would prefer to solve this as BlueZ-ish as possible

This seems like it would require a change to BlueZ, such as exposing a list of supported management APIs. Since Release can be called for multiple reasons, it doesn't seem like a reliable solution for detecting support of passive scanning (but it still does seem worth handling the call to Release since we have seen that not handling can lead to waiting for advertisements that will never come).

[bojanpotocnik]

I'll wait for #1146 and #1147 to be merged and then rebase using features from #1146.

[bdraco]

Would it work to detect "org.bluez.AdvertisementMonitorManager1" in the adapter details for this case?

https://github.com/Bluetooth-Devices/bluetooth-adapters/blob/bcadadd67150bd4648a0002dc151adc46d302732/src/bluetooth_adapters/systems/linux.py#L104

[bojanpotocnik]

Thank you for the suggestion, but unfortunately this would not solve this particular issue.

The issue here is that not only that org.bluez.AdvertisementMonitorManager1 is present in managed objects, even calling org.bluez.AdvertisementMonitorManager1.RegisterMonitor() succeeds. Only "later" kernel responds with Unknown Command (0x01) to underlying MGMT command Add Advertisement Patterns Monitor (0x0052) - this response is processed by the BlueZ code and registered AdvertisementMonitor1 is destroyed, but the error is not propagated to the D-Bus caller (bleak) in any way.

[bojanpotocnik]

Rebased on #1146.

[bojanpotocnik]

I've again hit a roadblock and would be happy for some ideas on how to solve it. So, there are many reasons why active discovery could be (prematurely) stopped (Discovering set to False), but only a specific case when Advertising Monitor is released.
I want to use this fact to detect that the passive scanning is not supported, instead of just informing the user that something stopped the discovery (e.g. using HCI injection or mgmt commands) and it should be restarted (which would not help in this case).
Trying to achieve that BleakNoPassiveScanError would be raised (preferably in the start() method), I noticed that I came really close to the previous solution, where I "processed the D-Bus events" after exporting the Advertising Manager 🤔

[bojanpotocnik]

This comment belongs to older implementation, available here https://github.com/bojanpotocnik/bleak/blob/archive/develop/1136_verify_adding_adv_monitor/bleak/backends/bluezdbus/manager.py#L487

[bojanpotocnik]

Based on the BlueZ tests, it seems like Release() being invoked for failed registration of the monitor is expected behaviour.

https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/test/example-adv-monitor#n328

    # Register the app root path to expose advertisement monitors.
    # Release() should get called on monitor0 - incorrect monitor type.
    # Activate() should get called on monitor1.
    ret = app.register_app()
    if not ret:
        print('RegisterMonitor failed.')
        mainloop.quit()
        exit(-1)

[bojanpotocnik]

After further checking BlueZ sources and tests, I used the fact that either Activate or Release will be invoked when the advertisement monitor is processed and came up with the current implementation.
One additional benefit of this implementation is that when BleakScanner.start() returns, we are sure that the passive scanning has actually been started (or failed).

[bojanpotocnik]

We can also keep the discovery_stopped_callback functionality, if required, by adding an intermediate callback which will call put_nowait and then discovery_stopped_callback.

[bojanpotocnik]

As the latest solution does not really use features from #1146, except adding a comment explaining why callback is not added to _discovery_stopped_callbacks when passive scanning

BlueZManager -> async passive_scan() -> async with self._bus_lock:
            self._device_removed_callbacks.append(device_removed_callback_and_state)

            # Once a monitoring job is activated by BlueZ, the client can expect to get notified
            # on the targeted advertisements no matter if there is an ongoing discovery session
            # (started/stopped with org.bluez.Adapter1.StartDiscovery/StopDiscovery, as done when
            # using active scanning).
            # That is why discovery_stopped_callback is not added to self._discovery_stopped_callbacks
            # at this point, as org.bluez.Adapter1.Discovering status is not relevant for passive
            # scanning.

            # If advertisement monitor is released before the scanning is stopped, it means that the

I've moved it to bojanpotocnik:1136_verify_adding_adv_monitor_rebased_1146 and rebased this one on develop.