Python based device scanning interfacing with native C++ code on linux. #4960

andy31415 · 2021-02-22T16:07:42Z

Problem

Native C++ device scanning is not implemented nor accessible from python, which does not allow us to verify/script the parsing of discovered devices.

Current discovery is done natively by Android and iOS platforms and a pure python version. However connectivity itself is done by C++ native code which would only be exercised on connect and never on discovery.

Summary of Changes

This is a merge of several branches developed while the bluez code split was under review. It contains:

Moved BlueZ main loop to a separate class (rather than inline method) so that it is accessible by other implementations
Implemented a 'CHIPDeviceScanner' that can be used both externally and is used internally by the 'connect via BLE' implementation in linux BLEManager
Added native handle classes for python for managing native interface calls and doing 'init' for platform-specific stack bits (e.g. memory and device layer initialization required by BLE)
Added bindings to the device scanner from python
Added more type safety to BlueZ code (less void *, replaced with actual types and avoided casting)

Example usage:

In [1]: import chip.ble

In [2]: chip.ble.DiscoverAsync(2000, lambda *x: print('Discovered: %r' % (x,)), lambda: print('Done'))
CHIP:DL: Platform main loop started.
CHIP:DL: TRACE: Bluez mainloop starting Thread
CHIP:BLE: BLE scanning through known devices.
CHIP:BLE: BLE initiating scan.

In [3]: CHIP:BLE: Device 38:CB:9F:C4:D5:7A does not look like a CHIP device.
CHIP:BLE: Device 79:1C:AB:8D:B4:1E does not look like a CHIP device.
CHIP:BLE: Device 6E:91:AB:C1:8B:75 does not look like a CHIP device.
CHIP:BLE: Device 53:DA:56:14:DF:2E does not look like a CHIP device.
Discovered: (b'98:F4:AB:6B:1A:FE', 3840, 65279, 9050)
Done
In [3]:

Moved Bluez mainloop into a separate class Moved blezl iterators into separate files Added ble generic initialization and handle management Switched bluez to use the newly developed scanner main loop.

todo · 2021-02-22T16:07:50Z

Factor common code in the two function.

connectedhomeip/src/platform/Linux/bluez/Helper.cpp

Lines 904 to 914 in 67b1525

    
           // TODO: Factor common code in the two function. 
        
           BluezConnection * conn; 
        
           VerifyOrExit(bluez_device1_get_connected(device), ChipLogError(DeviceLayer, "FAIL: device is not connected")); 
        
           conn = static_cast<BluezConnection *>( 
        
               g_hash_table_lookup(apEndpoint->mpConnMap, g_dbus_proxy_get_object_path(G_DBUS_PROXY(device)))); 
        
           VerifyOrExit(conn == nullptr, 
        
                        ChipLogError(DeviceLayer, "FAIL: connection already tracked: conn: %x new device: %s", conn, 
        
                                     g_dbus_proxy_get_object_path(G_DBUS_PROXY(device)))); 
        
           conn                = g_new0(BluezConnection, 1);

This comment was generated by todo based on a `TODO` comment in `67b1525` in #4960. cc @andy31415.

msandstedt · 2021-02-22T22:42:21Z

src/platform/Linux/bluez/MainLoop.cpp

+            g_free(mBluezMainLoop);
+            mBluezMainLoop = nullptr;
+        }
+    }


This cleanup logic looks suspicious. err will always equal CHIP_NO_ERROR, so this block can never execute. But if it was to execute, shouldn't the pthread also be cleaned up?

Fixed - I did not notice no error was ever set; thank you for catching.

mspang · 2021-02-24T00:38:47Z

src/controller/python/chip/ble/LinuxImpl.cpp

+
+private:
+    ChipDeviceScanner::Ptr mScanner;
+    PyObject * mContext;


PyObject * const mContext ?

Updated. Made me not be able to set it to nullptr in destructor (I was trying too set everything to null to catch any odd reference errors while debugging what turned out to be python void* cast issues).

mspang · 2021-02-24T00:39:01Z

src/controller/python/chip/ble/LinuxImpl.cpp

+
+    ~ScannerDelegateImpl()
+    {
+        mContext          = nullptr;


What's the purpose of assigning in the dtor?

Debugging invalid memory references. Turned out to be python c_void_p behavind oddly (it casts to 32-bit if it can, but when passing back it stays 32-bit int and does a sign extension or invalid high order bits when sent as a function argument ... very frustrating)

src/controller/python/chip/ble/types.py

src/controller/python/chip/ble/scan_devices.py

src/controller/python/chip/native/StackInit.cpp

mspang · 2021-02-24T00:58:19Z

src/platform/Linux/bluez/BluezObjectList.h

+    {
+        if (manager == nullptr)
+        {
+            ChipLogError(DeviceLayer, "Manager is NULL in %s", __func__);


Could this be an assert (manager != nullptr) ?

In general, I don't think "internal pointer is null" is ever an appropriate log message (except perhaps when debugging locally).

We should instead log something that describes the cause of the pointer being null, or not get into here in the first place.

I just moved this code (copied and pasted as is). Agree it would require some more passes, but I am afraid of going to deep if trying to update everything. The PR is already huge.

mspang · 2021-02-24T01:00:50Z

src/platform/Linux/bluez/ChipDeviceScanner.cpp

+namespace Internal {
+namespace {
+
+struct GObjectUnref


Can we use this in other cases where we're manually calling g_object_unref?

Eventually I assume yes. I think it should be pulled out more globally as we change this code more.

I wonder though how much we will still touch it: I wanted to have scanning and this PR has it. The only additonal thing we may want is BLE connection by address instead of discriminator.

src/platform/Linux/bluez/ChipDeviceScanner.h

src/platform/Linux/bluez/MainLoop.cpp

mspang · 2021-02-24T01:12:07Z

src/platform/Linux/bluez/Helper.cpp

-    BluezObjectsCleanup(endpoint);
+    if (!MainLoop::Instance().SetCleanupFunction(BluezObjectsCleanup, endpoint))
+    {
+        ChipLogError(DeviceLayer, "Failed to schedule cleanup function");


Why isn't this an assert?

Printing a message when things go badly off the rails isn't going to catch issues quickly and reliably (e.g., cause our tests to fail).

It may require additional passes: as it stands, we do not seem to ever stop the main loop, so I kept the cleanup function because cleanup existed in previous code, however this really does not get called in the current code structure.

src/platform/Linux/bluez/ChipDeviceScanner.h

…to be very useful

… library handle pointers to not use c_void_p directly. This looks stable on my linux machine (ran >100 scans)

…or wait

mspang · 2021-02-24T16:35:13Z

src/controller/python/chip/ble/LinuxImpl.cpp

+    }
+
+private:
+    std::unique_ptr<ChipDeviceScanner> mScanner;


Cannot do this because scanner is moved post object creation and I have a SetScanner method.

This is a side-effect of creation order: delegate needs to access the scanner, but scanner needs a delegate.

src/controller/python/chip/ble/LinuxImpl.cpp

src/controller/python/chip/ble/library_handle.py

src/platform/Linux/bluez/MainLoop.cpp

Large squashed change for python scanning support.

67b1525

Moved Bluez mainloop into a separate class Moved blezl iterators into separate files Added ble generic initialization and handle management Switched bluez to use the newly developed scanner main loop.

This comment has been minimized.

Sign in to view

boring-cyborg bot added controller linux platform labels Feb 22, 2021

pullapprove bot requested review from bzbarsky-apple, chrisdecenzo, hawk248, jelderton, msandstedt, mspang, rwalker-apple, sagar-apple, saurabhst and woody-apple February 22, 2021 16:08

Remove dedup todo: code is not duplicated anymore

606c330

project-chip deleted a comment from todo bot Feb 22, 2021

andy31415 added 2 commits February 22, 2021 11:10

Remove some fixmes for code that is unclear what next to implement

7b211f4

Add errno.h include due to code using errno

5f599ef

msandstedt reviewed Feb 22, 2021

View reviewed changes

woody-apple approved these changes Feb 23, 2021

View reviewed changes

mspang reviewed Feb 24, 2021

View reviewed changes

msandstedt reviewed Feb 24, 2021

View reviewed changes

src/platform/Linux/bluez/ChipDeviceScanner.h Outdated Show resolved Hide resolved

andy31415 added 2 commits February 24, 2021 09:18

Added support for "runAndWait" in the ble main loop - that one seems …

3095a63

…to be very useful

Update scanning to use blocking main scheduling for stopping , change…

876e113

… library handle pointers to not use c_void_p directly. This looks stable on my linux machine (ran >100 scans)

restyled-io bot mentioned this pull request Feb 24, 2021

Restyle Python based device scanning interfacing with native C++ code on linux. #4993

Closed

andy31415 added 3 commits February 24, 2021 09:35

Use wait for scheduling bluez adapter initialization

09b7a98

Code review comments: python docstrings, const on mContext

22d0a59

Drop _t from callback types in python

888e0c8

andy31415 added 2 commits February 24, 2021 10:02

Do not use Ptr as a shortcut to std::unique_ptr

f0edd38

Address code review: odd cleanup logic in main loop fixed

dfa5b2d

andy31415 requested review from msandstedt and mspang February 24, 2021 15:09

andy31415 added 3 commits February 24, 2021 10:35

Use semaphore to detect main loop running instead of pthread_yield

f031c7f

Set main loop as started right away, since we thread-start and post f…

46bc0e6

…or wait

Restyle fixes

7a936f8

mspang approved these changes Feb 24, 2021

View reviewed changes

msandstedt approved these changes Feb 24, 2021

View reviewed changes

andy31415 added 3 commits February 25, 2021 11:09

Address code review comments

d070c7f

Fix typo, use verify or die on context check in main loop

e1b5acc

Make it compile

30047a4

andy31415 merged commit 5ca7308 into project-chip:master Feb 25, 2021

todo bot mentioned this pull request Feb 25, 2021

Factor common code in the two function. #5022

Closed

msandstedt mentioned this pull request Mar 2, 2021

Add ability to redirect CHIP logs to a callback and add python log callback integration #5024

Merged

andy31415 deleted the python_ble_discovery_large branch October 28, 2021 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python based device scanning interfacing with native C++ code on linux. #4960

Python based device scanning interfacing with native C++ code on linux. #4960

andy31415 commented Feb 22, 2021 •

edited

Loading

This comment has been minimized.

todo bot commented Feb 22, 2021

msandstedt Feb 22, 2021

andy31415 Feb 24, 2021 •

edited

Loading

mspang Feb 24, 2021

andy31415 Feb 24, 2021

mspang Feb 24, 2021

andy31415 Feb 24, 2021

mspang Feb 24, 2021 •

edited

Loading

andy31415 Feb 24, 2021

mspang Feb 24, 2021

andy31415 Feb 24, 2021

mspang Feb 24, 2021

andy31415 Feb 25, 2021

mspang Feb 24, 2021

andy31415 Feb 25, 2021

Python based device scanning interfacing with native C++ code on linux. #4960

Python based device scanning interfacing with native C++ code on linux. #4960

Conversation

andy31415 commented Feb 22, 2021 • edited Loading

Problem

Summary of Changes

This comment has been minimized.

todo bot commented Feb 22, 2021

Factor common code in the two function.

This comment was generated by todo based on a TODO comment in 67b1525 in #4960. cc @andy31415.

Choose a reason for hiding this comment

andy31415 Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mspang Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy31415 commented Feb 22, 2021 •

edited

Loading

This comment was generated by todo based on a `TODO` comment in `67b1525` in #4960. cc @andy31415.

andy31415 Feb 24, 2021 •

edited

Loading

mspang Feb 24, 2021 •

edited

Loading