From b39e947b03ee8d95c5d7f921e39e01f08afc6ecd Mon Sep 17 00:00:00 2001 From: Justin Wood Date: Thu, 7 Jul 2022 15:36:27 -0700 Subject: [PATCH] [Python] Fix threading model (#20234) (#20465) * Fix Python Threading Model This fixes a number of issues with the Python REPL's threading model, namely: - Multiple, duplicated methods for stack initialization. - Stack initialization split across really odd places - Incorrect thread initialization - Incorrect order of initialization * Review feedback * Brought back the auto-stack init in native, albeit, a minimal version only. Updated ChipStack to now use native to implicitly achieve this auto init (which sets up MemoryInit) before continuining onwards with controller-style initialization. * Fix build Co-authored-by: Jerry Johns --- src/controller/python/BUILD.gn | 3 +- .../ChipDeviceController-ScriptBinding.cpp | 95 ++++++++-------- src/controller/python/chip/ChipStack.py | 86 +++++---------- .../python/chip/native/CommonStackInit.cpp | 61 +++++++++++ .../python/chip/native/StackInit.cpp | 102 ------------------ src/controller/python/chip/native/__init__.py | 10 +- 6 files changed, 148 insertions(+), 209 deletions(-) create mode 100644 src/controller/python/chip/native/CommonStackInit.cpp delete mode 100644 src/controller/python/chip/native/StackInit.cpp diff --git a/src/controller/python/BUILD.gn b/src/controller/python/BUILD.gn index 5b90275de38dc6..63332c5fe59071 100644 --- a/src/controller/python/BUILD.gn +++ b/src/controller/python/BUILD.gn @@ -44,6 +44,8 @@ shared_library("ChipDeviceCtrl") { "chip/setup_payload/Parser.cpp", ] + sources += [ "chip/native/CommonStackInit.cpp" ] + if (chip_controller) { sources += [ "ChipCommissionableNodeController-ScriptBinding.cpp", @@ -63,7 +65,6 @@ shared_library("ChipDeviceCtrl") { "chip/internal/ChipThreadWork.h", "chip/internal/CommissionerImpl.cpp", "chip/logging/LoggingRedirect.cpp", - "chip/native/StackInit.cpp", ] } else { sources += [ diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp index 0e2342e3eca833..04ffeb5fd1cf20 100644 --- a/src/controller/python/ChipDeviceController-ScriptBinding.cpp +++ b/src/controller/python/ChipDeviceController-ScriptBinding.cpp @@ -40,12 +40,6 @@ #include #include -#include "ChipDeviceController-ScriptDevicePairingDelegate.h" -#include "ChipDeviceController-StorageDelegate.h" - -#include "chip/interaction_model/Delegate.h" - -#include #include #include #include @@ -55,6 +49,11 @@ #include #include #include + +#include +#include +#include + #include #include #include @@ -71,6 +70,10 @@ #include #include +#include +#include +#include + using namespace chip; using namespace chip::Ble; using namespace chip::Controller; @@ -106,7 +109,8 @@ chip::NodeId kDefaultLocalDeviceId = chip::kTestControllerNodeId; chip::NodeId kRemoteDeviceId = chip::kTestDeviceNodeId; extern "C" { -ChipError::StorageType pychip_DeviceController_StackInit(); +ChipError::StorageType pychip_DeviceController_StackInit(uint32_t bluetoothAdapterId); +ChipError::StorageType pychip_DeviceController_StackShutdown(); ChipError::StorageType pychip_DeviceController_NewDeviceController(chip::Controller::DeviceCommissioner ** outDevCtrl, chip::NodeId localDeviceId, bool useTestCommissioner); @@ -176,8 +180,6 @@ ChipError::StorageType pychip_DeviceCommissioner_CloseBleConnection(chip::Contro uint8_t pychip_DeviceController_GetLogFilter(); void pychip_DeviceController_SetLogFilter(uint8_t category); -ChipError::StorageType pychip_Stack_Init(); -ChipError::StorageType pychip_Stack_Shutdown(); const char * pychip_Stack_ErrorToString(ChipError::StorageType err); const char * pychip_Stack_StatusReportToString(uint32_t profileId, uint16_t statusCode); void pychip_Stack_SetLogFunct(LogMessageFunct logFunct); @@ -187,8 +189,6 @@ ChipError::StorageType pychip_GetConnectedDeviceByNodeId(chip::Controller::Devic ChipError::StorageType pychip_GetDeviceBeingCommissioned(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeId, CommissioneeDeviceProxy ** proxy); uint64_t pychip_GetCommandSenderHandle(chip::DeviceProxy * device); -// CHIP Stack objects -ChipError::StorageType pychip_BLEMgrImpl_ConfigureBle(uint32_t bluetoothAdapterId); chip::ChipError::StorageType pychip_InteractionModel_ShutdownSubscription(SubscriptionId subscriptionId); @@ -220,10 +220,17 @@ chip::Controller::Python::StorageAdapter * pychip_Storage_GetStorageAdapter() return sStorageAdapter; } -ChipError::StorageType pychip_DeviceController_StackInit() +ChipError::StorageType pychip_DeviceController_StackInit(uint32_t bluetoothAdapterId) { VerifyOrDie(sStorageAdapter != nullptr); +#if CHIP_DEVICE_LAYER_TARGET_LINUX && CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE + // By default, Linux device is configured as a BLE peripheral while the controller needs a BLE central. + CHIP_ERROR err = + chip::DeviceLayer::Internal::BLEMgrImpl().ConfigureBle(/* BLE adapter ID */ bluetoothAdapterId, /* BLE central */ true); + VerifyOrReturnError(err == CHIP_NO_ERROR, err.AsInteger()); +#endif + FactoryInitParams factoryParams; factoryParams.fabricIndependentStorage = sStorageAdapter; @@ -236,6 +243,12 @@ ChipError::StorageType pychip_DeviceController_StackInit() factoryParams.enableServerInteractions = true; + // Hack needed due to the fact that DnsSd server uses the CommissionableDataProvider even + // when never starting commissionable advertising. This will not be used but prevents + // null pointer dereferences. + static chip::DeviceLayer::TestOnlyCommissionableDataProvider TestOnlyCommissionableDataProvider; + chip::DeviceLayer::SetCommissionableDataProvider(&TestOnlyCommissionableDataProvider); + ReturnErrorOnFailure(DeviceControllerFactory::GetInstance().Init(factoryParams).AsInteger()); // @@ -249,6 +262,32 @@ ChipError::StorageType pychip_DeviceController_StackInit() // DeviceControllerFactory::GetInstance().RetainSystemState(); + // + // Finally, start up the main Matter thread. Any further interactions with the stack + // will now need to happen on the Matter thread, OR protected with the stack lock. + // + ReturnErrorOnFailure(chip::DeviceLayer::PlatformMgr().StartEventLoopTask().AsInteger()); + + return CHIP_NO_ERROR.AsInteger(); +} + +ChipError::StorageType pychip_DeviceController_StackShutdown() +{ + ChipLogError(Controller, "Shutting down the stack..."); + + // + // Let's stop the Matter thread, and wait till the event loop has stopped. + // + ReturnErrorOnFailure(chip::DeviceLayer::PlatformMgr().StopEventLoopTask().AsInteger()); + + // + // There is the symmetric call to match the Retain called at stack initialization + // time. This will release all resources (if there are no other controllers active). + // + DeviceControllerFactory::GetInstance().ReleaseSystemState(); + + DeviceControllerFactory::GetInstance().Shutdown(); + return CHIP_NO_ERROR.AsInteger(); } @@ -555,38 +594,6 @@ ChipError::StorageType pychip_ScriptDevicePairingDelegate_SetCommissioningStatus return CHIP_NO_ERROR.AsInteger(); } -ChipError::StorageType pychip_Stack_Init() -{ - CHIP_ERROR err = CHIP_NO_ERROR; - - err = chip::Platform::MemoryInit(); - SuccessOrExit(err); - -#if !CHIP_SYSTEM_CONFIG_USE_SOCKETS - - ExitNow(err = CHIP_ERROR_NOT_IMPLEMENTED); - -#else /* CHIP_SYSTEM_CONFIG_USE_SOCKETS */ - -#endif /* CHIP_SYSTEM_CONFIG_USE_SOCKETS */ - -exit: - if (err != CHIP_NO_ERROR) - pychip_Stack_Shutdown(); - - return err.AsInteger(); -} - -ChipError::StorageType pychip_Stack_Shutdown() -{ - // - // There is the symmetric call to match the Retain called at stack initialization - // time. - // - DeviceControllerFactory::GetInstance().ReleaseSystemState(); - return CHIP_NO_ERROR.AsInteger(); -} - const char * pychip_Stack_ErrorToString(ChipError::StorageType err) { return chip::ErrorStr(CHIP_ERROR(err)); diff --git a/src/controller/python/chip/ChipStack.py b/src/controller/python/chip/ChipStack.py index 666d1192f7a9f8..ef1cd597d9ef9e 100644 --- a/src/controller/python/chip/ChipStack.py +++ b/src/controller/python/chip/ChipStack.py @@ -46,6 +46,8 @@ from .clusters import Objects as GeneratedObjects from .clusters.CHIPClusters import * +import chip.native + __all__ = [ "DeviceStatusStruct", "ChipStackException", @@ -182,7 +184,10 @@ def __init__(self, persistentStoragePath: str, installDefaultLogHandler=True, bl self._activeLogFunct = None self.addModulePrefixToLogMessage = True + # # Locate and load the chip shared library. + # This also implictly does a minimal stack initialization (i.e call MemoryInit). + # self._loadLib() # Arrange to log output from the chip library to a python logger object with the @@ -247,22 +252,17 @@ def HandleChipThreadRun(callback): # set by other modules(BLE) that require service by thread while thread blocks. self.blockingCB = None - # Initialize the chip library - res = self._ChipStackLib.pychip_Stack_Init() - if res != 0: - raise self.ErrorToException(res) + # + # Storage has to be initialized BEFORE initializing the stack, since the latter + # requires a PersistentStorageDelegate to be provided to DeviceControllerFactory. + # + self._persistentStorage = PersistentStorage(persistentStoragePath) if (bluetoothAdapter is None): bluetoothAdapter = 0 - res = self._ChipStackLib.pychip_BLEMgrImpl_ConfigureBle( - bluetoothAdapter) - if res != 0: - raise self.ErrorToException(res) - - self._persistentStorage = PersistentStorage(persistentStoragePath) - - res = self._ChipStackLib.pychip_DeviceController_StackInit() + # Initialize the chip stack. + res = self._ChipStackLib.pychip_DeviceController_StackInit(bluetoothAdapter) if res != 0: raise self.ErrorToException(res) @@ -321,7 +321,13 @@ def Shutdown(self): # Make sure PersistentStorage is destructed before chipStack # to avoid accessing builtins.chipStack after destruction. self._persistentStorage = None - self.Call(lambda: self._ChipStackLib.pychip_Stack_Shutdown()) + self.Call(lambda: self._ChipStackLib.pychip_DeviceController_StackShutdown()) + + # + # Stack init happens in native, but shutdown happens here unfortunately. + # #20437 tracks consolidating these. + # + self._ChipStackLib.pychip_CommonStackShutdown() self.networkLock = None self.completeEvent = None self._ChipStackLib = None @@ -409,42 +415,8 @@ def ErrorToException(self, err, devStatusPtr=None): ) def LocateChipDLL(self): - if self._chipDLLPath: - return self._chipDLLPath - - scriptDir = os.path.dirname(os.path.abspath(__file__)) - - # When properly installed in the chip package, the Chip Device Manager DLL will - # be located in the package root directory, along side the package's - # modules. - dmDLLPath = os.path.join(scriptDir, ChipStackDLLBaseName) - if os.path.exists(dmDLLPath): - self._chipDLLPath = dmDLLPath - return self._chipDLLPath - - # For the convenience of developers, search the list of parent paths relative to the - # running script looking for an CHIP build directory containing the Chip Device - # Manager DLL. This makes it possible to import and use the ChipDeviceMgr module - # directly from a built copy of the CHIP source tree. - buildMachineGlob = "%s-*-%s*" % (platform.machine(), - platform.system().lower()) - relDMDLLPathGlob = os.path.join( - "build", - buildMachineGlob, - "src/controller/python/.libs", - ChipStackDLLBaseName, - ) - for dir in self._AllDirsToRoot(scriptDir): - dmDLLPathGlob = os.path.join(dir, relDMDLLPathGlob) - for dmDLLPath in glob.glob(dmDLLPathGlob): - if os.path.exists(dmDLLPath): - self._chipDLLPath = dmDLLPath - return self._chipDLLPath - - raise Exception( - "Unable to locate Chip Device Manager DLL (%s); expected location: %s" - % (ChipStackDLLBaseName, scriptDir) - ) + self._loadLib() + return self._chipDLLPath # ----- Private Members ----- def _AllDirsToRoot(self, dir): @@ -458,11 +430,13 @@ def _AllDirsToRoot(self, dir): def _loadLib(self): if self._ChipStackLib is None: - self._ChipStackLib = CDLL(self.LocateChipDLL()) - self._ChipStackLib.pychip_Stack_Init.argtypes = [] - self._ChipStackLib.pychip_Stack_Init.restype = c_uint32 - self._ChipStackLib.pychip_Stack_Shutdown.argtypes = [] - self._ChipStackLib.pychip_Stack_Shutdown.restype = c_uint32 + self._ChipStackLib = chip.native.GetLibraryHandle() + self._chipDLLPath = chip.native.FindNativeLibraryPath() + + self._ChipStackLib.pychip_DeviceController_StackInit.argtypes = [c_uint32] + self._ChipStackLib.pychip_DeviceController_StackInit.restype = c_uint32 + self._ChipStackLib.pychip_DeviceController_StackShutdown.argtypes = [] + self._ChipStackLib.pychip_DeviceController_StackShutdown.restype = c_uint32 self._ChipStackLib.pychip_Stack_StatusReportToString.argtypes = [ c_uint32, c_uint16, @@ -474,10 +448,6 @@ def _loadLib(self): _LogMessageFunct] self._ChipStackLib.pychip_Stack_SetLogFunct.restype = c_uint32 - self._ChipStackLib.pychip_BLEMgrImpl_ConfigureBle.argtypes = [ - c_uint32] - self._ChipStackLib.pychip_BLEMgrImpl_ConfigureBle.restype = c_uint32 - self._ChipStackLib.pychip_DeviceController_PostTaskOnChipThread.argtypes = [ _ChipThreadTaskRunnerFunct, py_object] self._ChipStackLib.pychip_DeviceController_PostTaskOnChipThread.restype = c_uint32 diff --git a/src/controller/python/chip/native/CommonStackInit.cpp b/src/controller/python/chip/native/CommonStackInit.cpp new file mode 100644 index 00000000000000..5fa470713b2079 --- /dev/null +++ b/src/controller/python/chip/native/CommonStackInit.cpp @@ -0,0 +1,61 @@ +/* + * + * Copyright (c) 2020-2022 Project CHIP Authors + * Copyright (c) 2019-2020 Google LLC. + * Copyright (c) 2013-2018 Nest Labs, Inc. + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * @file + * Implementation of the native methods expected by the Python + * version of Chip Device Manager. + * + */ + +#include +#include + +#include +#include + +#include +#include +#include +#include +#include +#include + +static_assert(std::is_same::value, "python assumes CHIP_ERROR maps to c_uint32"); + +extern "C" { + +chip::ChipError::StorageType pychip_CommonStackInit() +{ + ReturnErrorOnFailure(chip::Platform::MemoryInit().AsInteger()); + ReturnErrorOnFailure(chip::DeviceLayer::PlatformMgr().InitChipStack().AsInteger()); + return CHIP_NO_ERROR.AsInteger(); +} + +void pychip_CommonStackShutdown() +{ +#if 0 // + // We cannot actually call this because the destructor for the MdnsContexts singleton on Darwin only gets called + // on termination of the program, and that unfortunately makes a bunch of Platform::MemoryFree calls. + // + chip::Platform::MemoryShutdown(); +#endif +} +}; diff --git a/src/controller/python/chip/native/StackInit.cpp b/src/controller/python/chip/native/StackInit.cpp deleted file mode 100644 index 4b140899b43cb2..00000000000000 --- a/src/controller/python/chip/native/StackInit.cpp +++ /dev/null @@ -1,102 +0,0 @@ -/* - * - * Copyright (c) 2021 Project CHIP Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -#include -#include - -#include -#include -#include -#include -#include -#include -#include - -namespace { - -pthread_t sPlatformMainThread; -#if CHIP_DEVICE_LAYER_TARGET_LINUX && CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE -uint32_t sBluetoothAdapterId = 0; -#endif - -void * PlatformMainLoop(void *) -{ - ChipLogProgress(DeviceLayer, "Platform main loop started."); - chip::DeviceLayer::PlatformMgr().RunEventLoop(); - ChipLogProgress(DeviceLayer, "Platform main loop completed."); - return nullptr; -} - -} // namespace - -extern "C" { - -static_assert(std::is_same::value, "python assumes CHIP_ERROR maps to c_uint32"); - -chip::ChipError::StorageType pychip_BLEMgrImpl_ConfigureBle(uint32_t bluetoothAdapterId) -{ -#if CHIP_DEVICE_LAYER_TARGET_LINUX && CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE - // By default, Linux device is configured as a BLE peripheral while the controller needs a BLE central. - sBluetoothAdapterId = bluetoothAdapterId; - CHIP_ERROR err = - chip::DeviceLayer::Internal::BLEMgrImpl().ConfigureBle(/* BLE adapter ID */ bluetoothAdapterId, /* BLE central */ true); - VerifyOrReturnError(err == CHIP_NO_ERROR, err.AsInteger()); -#endif - return CHIP_NO_ERROR.AsInteger(); -} - -void pychip_native_init() -{ - CHIP_ERROR err = CHIP_NO_ERROR; - - err = chip::Platform::MemoryInit(); - if (err != CHIP_NO_ERROR) - { - ChipLogError(DeviceLayer, "Failed to initialize CHIP stack: memory init failed: %s", chip::ErrorStr(err)); - } - -#if CHIP_DEVICE_LAYER_TARGET_LINUX && CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE - // By default, Linux device is configured as a BLE peripheral while the controller needs a BLE central. - err = chip::DeviceLayer::Internal::BLEMgrImpl().ConfigureBle(/* BLE adapter ID */ sBluetoothAdapterId, /* BLE central */ true); - if (err != CHIP_NO_ERROR) - { - ChipLogError(DeviceLayer, "Failed to configure BLE central: %s", chip::ErrorStr(err)); - } -#endif // CHIP_DEVICE_LAYER_TARGET_LINUX && CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE - - err = chip::DeviceLayer::PlatformMgr().InitChipStack(); - if (err != CHIP_NO_ERROR) - { - ChipLogError(DeviceLayer, "Failed to initialize CHIP stack: platform init failed: %s", chip::ErrorStr(err)); - } - - // Hack needed due to the fact that DnsSd server uses the CommissionableDataProvider even - // when never starting operational advertising. This will not be used but prevents - // null pointer dereferences. - static chip::DeviceLayer::TestOnlyCommissionableDataProvider TestOnlyCommissionableDataProvider; - chip::DeviceLayer::SetCommissionableDataProvider(&TestOnlyCommissionableDataProvider); - - int result = pthread_create(&sPlatformMainThread, nullptr, PlatformMainLoop, nullptr); -#if CHIP_ERROR_LOGGING - int tmpErrno = errno; -#endif // CHIP_ERROR_LOGGING - - if (result != 0) - { - ChipLogError(DeviceLayer, "Failed to initialize CHIP stack: pthread_create failed: %s", strerror(tmpErrno)); - } -} -} diff --git a/src/controller/python/chip/native/__init__.py b/src/controller/python/chip/native/__init__.py index 19ad4466ef8220..44b4ccd89f015e 100644 --- a/src/controller/python/chip/native/__init__.py +++ b/src/controller/python/chip/native/__init__.py @@ -75,11 +75,13 @@ def GetLibraryHandle() -> ctypes.CDLL: global _nativeLibraryHandle if _nativeLibraryHandle is None: _nativeLibraryHandle = ctypes.CDLL(FindNativeLibraryPath()) - setter = NativeLibraryHandleMethodArguments(_nativeLibraryHandle) + setter.Set("pychip_CommonStackInit", ctypes.c_uint32, []) - setter.Set("pychip_native_init", None, []) - - _nativeLibraryHandle.pychip_native_init() + # + # We've a split initialization model with some init happening here and some other + # bits of init happening in ChipStack. #20437 tracks consolidating those. + # + _nativeLibraryHandle.pychip_CommonStackInit() return _nativeLibraryHandle