From 36a4feab5724c1ca49aa157b7895f0e19c83e9db Mon Sep 17 00:00:00 2001 From: Jerry Johns Date: Fri, 1 Jul 2022 12:44:49 -0700 Subject: [PATCH] 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 --- src/controller/python/BUILD.gn | 1 - .../ChipDeviceController-ScriptBinding.cpp | 90 ++++++++++------ src/controller/python/chip/ChipStack.py | 26 ++--- .../python/chip/native/StackInit.cpp | 102 ------------------ src/controller/python/chip/native/__init__.py | 5 - 5 files changed, 69 insertions(+), 155 deletions(-) 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..6016d58957469c 100644 --- a/src/controller/python/BUILD.gn +++ b/src/controller/python/BUILD.gn @@ -63,7 +63,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 3c56dc730cac53..a9016ec0e5b6a4 100644 --- a/src/controller/python/ChipDeviceController-ScriptBinding.cpp +++ b/src/controller/python/ChipDeviceController-ScriptBinding.cpp @@ -71,6 +71,10 @@ #include #include +#include +#include +#include + using namespace chip; using namespace chip::Ble; using namespace chip::Controller; @@ -107,6 +111,7 @@ chip::NodeId kRemoteDeviceId = chip::kTestDeviceNodeId; extern "C" { ChipError::StorageType pychip_DeviceController_StackInit(); +ChipError::StorageType pychip_DeviceController_StackShutdown(); ChipError::StorageType pychip_DeviceController_NewDeviceController(chip::Controller::DeviceCommissioner ** outDevCtrl, chip::NodeId localDeviceId, bool useTestCommissioner); @@ -176,8 +181,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); @@ -222,6 +225,15 @@ chip::Controller::Python::StorageAdapter * pychip_Storage_GetStorageAdapter() ChipError::StorageType pychip_DeviceController_StackInit() { + ReturnErrorOnFailure(chip::Platform::MemoryInit().AsInteger()); + + auto err = chip::DeviceLayer::PlatformMgr().InitChipStack(); + if (err != CHIP_NO_ERROR) + { + ChipLogError(DeviceLayer, "Failed to initialize CHIP stack: platform init failed: %s", chip::ErrorStr(err)); + return err.AsInteger(); + } + VerifyOrDie(sStorageAdapter != nullptr); FactoryInitParams factoryParams; @@ -236,6 +248,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 operational 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 +267,30 @@ 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() +{ + // + // 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(); + + chip::DeviceLayer::PlatformMgr().Shutdown(); + return CHIP_NO_ERROR.AsInteger(); } @@ -313,6 +355,18 @@ void pychip_DeviceController_SetLogFilter(uint8_t category) #endif } +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. + 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(); +} + ChipError::StorageType pychip_DeviceController_ConnectBLE(chip::Controller::DeviceCommissioner * devCtrl, uint16_t discriminator, uint32_t setupPINCode, chip::NodeId nodeid) { @@ -557,38 +611,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..d43bd923fcb4f4 100644 --- a/src/controller/python/chip/ChipStack.py +++ b/src/controller/python/chip/ChipStack.py @@ -247,8 +247,14 @@ 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() + # + # Storage has to be initialized BEFORE initializing the stack, since the latter + # requires a PersistentStorageDelegate to be provided to DeviceControllerFactory. + # + self._persistentStorage = PersistentStorage(persistentStoragePath) + + # Initialize the chip stack. + res = self._ChipStackLib.pychip_DeviceController_StackInit() if res != 0: raise self.ErrorToException(res) @@ -260,12 +266,6 @@ def HandleChipThreadRun(callback): if res != 0: raise self.ErrorToException(res) - self._persistentStorage = PersistentStorage(persistentStoragePath) - - res = self._ChipStackLib.pychip_DeviceController_StackInit() - if res != 0: - raise self.ErrorToException(res) - im.InitIMDelegate() ClusterAttribute.Init() ClusterCommand.Init() @@ -321,7 +321,7 @@ 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()) self.networkLock = None self.completeEvent = None self._ChipStackLib = None @@ -459,10 +459,10 @@ 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.pychip_DeviceController_StackInit.argtypes = [] + 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, 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..b34bb0adb1f4a0 100644 --- a/src/controller/python/chip/native/__init__.py +++ b/src/controller/python/chip/native/__init__.py @@ -75,11 +75,6 @@ def GetLibraryHandle() -> ctypes.CDLL: global _nativeLibraryHandle if _nativeLibraryHandle is None: _nativeLibraryHandle = ctypes.CDLL(FindNativeLibraryPath()) - setter = NativeLibraryHandleMethodArguments(_nativeLibraryHandle) - setter.Set("pychip_native_init", None, []) - - _nativeLibraryHandle.pychip_native_init() - return _nativeLibraryHandle