From 36a4feab5724c1ca49aa157b7895f0e19c83e9db Mon Sep 17 00:00:00 2001 From: Jerry Johns Date: Fri, 1 Jul 2022 12:44:49 -0700 Subject: [PATCH 1/4] 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 From c9b6e0c38d92926cff99b8b9879ebd5d28015192 Mon Sep 17 00:00:00 2001 From: Jerry Johns Date: Thu, 7 Jul 2022 07:04:31 -0700 Subject: [PATCH 2/4] Review feedback --- src/controller/python/BUILD.gn | 2 + .../ChipDeviceController-ScriptBinding.cpp | 52 ++++++-------- src/controller/python/CommonStackInit.cpp | 67 +++++++++++++++++++ src/controller/python/CommonStackInit.h | 47 +++++++++++++ src/controller/python/chip/ChipStack.py | 15 +---- 5 files changed, 140 insertions(+), 43 deletions(-) create mode 100644 src/controller/python/CommonStackInit.cpp create mode 100644 src/controller/python/CommonStackInit.h diff --git a/src/controller/python/BUILD.gn b/src/controller/python/BUILD.gn index 6016d58957469c..d9cc0dd4fd3d6a 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 += [ "CommonStackInit.cpp" ] + if (chip_controller) { sources += [ "ChipCommissionableNodeController-ScriptBinding.cpp", diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp index a9016ec0e5b6a4..2d54e6fcf7cd09 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,12 @@ #include #include #include + +#include +#include +#include +#include + #include #include #include @@ -110,7 +110,7 @@ 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, @@ -190,8 +190,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); @@ -223,19 +221,19 @@ chip::Controller::Python::StorageAdapter * pychip_Storage_GetStorageAdapter() return sStorageAdapter; } -ChipError::StorageType pychip_DeviceController_StackInit() +ChipError::StorageType pychip_DeviceController_StackInit(uint32_t bluetoothAdapterId) { - 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(); - } + ReturnErrorOnFailure(chip::Controller::Python::CommonStackInit().AsInteger()); 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; @@ -278,6 +276,8 @@ ChipError::StorageType pychip_DeviceController_StackInit() 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. // @@ -289,7 +289,9 @@ ChipError::StorageType pychip_DeviceController_StackShutdown() // DeviceControllerFactory::GetInstance().ReleaseSystemState(); - chip::DeviceLayer::PlatformMgr().Shutdown(); + DeviceControllerFactory::GetInstance().Shutdown(); + + chip::Controller::Python::CommonStackShutdown(); return CHIP_NO_ERROR.AsInteger(); } @@ -355,18 +357,6 @@ 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) { diff --git a/src/controller/python/CommonStackInit.cpp b/src/controller/python/CommonStackInit.cpp new file mode 100644 index 00000000000000..7bfdb860e04531 --- /dev/null +++ b/src/controller/python/CommonStackInit.cpp @@ -0,0 +1,67 @@ +/* + * + * 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 + +namespace chip { +namespace Controller { +namespace Python { + +CHIP_ERROR CommonStackInit() +{ + ReturnErrorOnFailure(chip::Platform::MemoryInit()); + ReturnErrorOnFailure(chip::DeviceLayer::PlatformMgr().InitChipStack()); + return CHIP_NO_ERROR; +} + +void CommonStackShutdown() +{ + chip::DeviceLayer::PlatformMgr().Shutdown(); + +#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 +} + +} // namespace Python +} // namespace Controller +} // namespace chip diff --git a/src/controller/python/CommonStackInit.h b/src/controller/python/CommonStackInit.h new file mode 100644 index 00000000000000..a28f4163ee5e0f --- /dev/null +++ b/src/controller/python/CommonStackInit.h @@ -0,0 +1,47 @@ +/* + * + * 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 + +namespace chip { +namespace Controller { +namespace Python { + +// +// This encapsulates stack initialization logic that is common to both +// controller and server initialization sequences. +// +CHIP_ERROR CommonStackInit(); + +// +// This encapsulates stack shutdown logic that is common to both +// controller and server shutdown sequences. +// +void CommonStackShutdown(); + +} // namespace Python +} // namespace Controller +} // namespace chip diff --git a/src/controller/python/chip/ChipStack.py b/src/controller/python/chip/ChipStack.py index d43bd923fcb4f4..f49f28d85bfe4e 100644 --- a/src/controller/python/chip/ChipStack.py +++ b/src/controller/python/chip/ChipStack.py @@ -253,16 +253,11 @@ def HandleChipThreadRun(callback): # self._persistentStorage = PersistentStorage(persistentStoragePath) - # Initialize the chip stack. - res = self._ChipStackLib.pychip_DeviceController_StackInit() - if res != 0: - raise self.ErrorToException(res) - if (bluetoothAdapter is None): bluetoothAdapter = 0 - res = self._ChipStackLib.pychip_BLEMgrImpl_ConfigureBle( - bluetoothAdapter) + # Initialize the chip stack. + res = self._ChipStackLib.pychip_DeviceController_StackInit(bluetoothAdapter) if res != 0: raise self.ErrorToException(res) @@ -459,7 +454,7 @@ def _AllDirsToRoot(self, dir): def _loadLib(self): if self._ChipStackLib is None: self._ChipStackLib = CDLL(self.LocateChipDLL()) - self._ChipStackLib.pychip_DeviceController_StackInit.argtypes = [] + 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 @@ -474,10 +469,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 From fa7ac2fe9aa1377364060c039234d3ac631c3b1b Mon Sep 17 00:00:00 2001 From: Jerry Johns Date: Thu, 7 Jul 2022 09:14:20 -0700 Subject: [PATCH 3/4] 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. --- src/controller/python/BUILD.gn | 2 +- .../ChipDeviceController-ScriptBinding.cpp | 7 +-- src/controller/python/CommonStackInit.h | 47 ---------------- src/controller/python/chip/ChipStack.py | 53 ++++++------------- .../{ => chip/native}/CommonStackInit.cpp | 19 +++---- src/controller/python/chip/native/__init__.py | 7 +++ 6 files changed, 31 insertions(+), 104 deletions(-) delete mode 100644 src/controller/python/CommonStackInit.h rename src/controller/python/{ => chip/native}/CommonStackInit.cpp (85%) diff --git a/src/controller/python/BUILD.gn b/src/controller/python/BUILD.gn index d9cc0dd4fd3d6a..63332c5fe59071 100644 --- a/src/controller/python/BUILD.gn +++ b/src/controller/python/BUILD.gn @@ -44,7 +44,7 @@ shared_library("ChipDeviceCtrl") { "chip/setup_payload/Parser.cpp", ] - sources += [ "CommonStackInit.cpp" ] + sources += [ "chip/native/CommonStackInit.cpp" ] if (chip_controller) { sources += [ diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp index 2d54e6fcf7cd09..6fc01943177c7d 100644 --- a/src/controller/python/ChipDeviceController-ScriptBinding.cpp +++ b/src/controller/python/ChipDeviceController-ScriptBinding.cpp @@ -52,7 +52,6 @@ #include #include -#include #include #include @@ -223,8 +222,6 @@ chip::Controller::Python::StorageAdapter * pychip_Storage_GetStorageAdapter() ChipError::StorageType pychip_DeviceController_StackInit(uint32_t bluetoothAdapterId) { - ReturnErrorOnFailure(chip::Controller::Python::CommonStackInit().AsInteger()); - VerifyOrDie(sStorageAdapter != nullptr); #if CHIP_DEVICE_LAYER_TARGET_LINUX && CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE @@ -247,7 +244,7 @@ ChipError::StorageType pychip_DeviceController_StackInit(uint32_t bluetoothAdapt 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 + // when never starting commissionable advertising. This will not be used but prevents // null pointer dereferences. static chip::DeviceLayer::TestOnlyCommissionableDataProvider TestOnlyCommissionableDataProvider; chip::DeviceLayer::SetCommissionableDataProvider(&TestOnlyCommissionableDataProvider); @@ -291,8 +288,6 @@ ChipError::StorageType pychip_DeviceController_StackShutdown() DeviceControllerFactory::GetInstance().Shutdown(); - chip::Controller::Python::CommonStackShutdown(); - return CHIP_NO_ERROR.AsInteger(); } diff --git a/src/controller/python/CommonStackInit.h b/src/controller/python/CommonStackInit.h deleted file mode 100644 index a28f4163ee5e0f..00000000000000 --- a/src/controller/python/CommonStackInit.h +++ /dev/null @@ -1,47 +0,0 @@ -/* - * - * 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 - -namespace chip { -namespace Controller { -namespace Python { - -// -// This encapsulates stack initialization logic that is common to both -// controller and server initialization sequences. -// -CHIP_ERROR CommonStackInit(); - -// -// This encapsulates stack shutdown logic that is common to both -// controller and server shutdown sequences. -// -void CommonStackShutdown(); - -} // namespace Python -} // namespace Controller -} // namespace chip diff --git a/src/controller/python/chip/ChipStack.py b/src/controller/python/chip/ChipStack.py index f49f28d85bfe4e..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 @@ -317,6 +322,12 @@ def Shutdown(self): # to avoid accessing builtins.chipStack after destruction. self._persistentStorage = None 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 @@ -404,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): @@ -453,7 +430,9 @@ def _AllDirsToRoot(self, dir): def _loadLib(self): if self._ChipStackLib is None: - self._ChipStackLib = CDLL(self.LocateChipDLL()) + 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 = [] diff --git a/src/controller/python/CommonStackInit.cpp b/src/controller/python/chip/native/CommonStackInit.cpp similarity index 85% rename from src/controller/python/CommonStackInit.cpp rename to src/controller/python/chip/native/CommonStackInit.cpp index 7bfdb860e04531..a6a9b3e455899e 100644 --- a/src/controller/python/CommonStackInit.cpp +++ b/src/controller/python/chip/native/CommonStackInit.cpp @@ -25,8 +25,6 @@ * */ -#include - #include #include @@ -39,21 +37,19 @@ #include #include -namespace chip { -namespace Controller { -namespace Python { +static_assert(std::is_same::value, "python assumes CHIP_ERROR maps to c_uint32"); + +extern "C" { -CHIP_ERROR CommonStackInit() +CHIP_ERROR pychip_CommonStackInit() { ReturnErrorOnFailure(chip::Platform::MemoryInit()); ReturnErrorOnFailure(chip::DeviceLayer::PlatformMgr().InitChipStack()); return CHIP_NO_ERROR; } -void CommonStackShutdown() +void pychip_CommonStackShutdown() { - chip::DeviceLayer::PlatformMgr().Shutdown(); - #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. @@ -61,7 +57,4 @@ void CommonStackShutdown() chip::Platform::MemoryShutdown(); #endif } - -} // namespace Python -} // namespace Controller -} // namespace chip +}; diff --git a/src/controller/python/chip/native/__init__.py b/src/controller/python/chip/native/__init__.py index b34bb0adb1f4a0..e147f684f605e3 100644 --- a/src/controller/python/chip/native/__init__.py +++ b/src/controller/python/chip/native/__init__.py @@ -76,5 +76,12 @@ def GetLibraryHandle() -> ctypes.CDLL: if _nativeLibraryHandle is None: _nativeLibraryHandle = ctypes.CDLL(FindNativeLibraryPath()) setter = NativeLibraryHandleMethodArguments(_nativeLibraryHandle) + setter.Set("pychip_CommonStackInit", None, []) + + # + # 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 From e6a7b2bd2261730c4cf70ba2a432407dda790cfe Mon Sep 17 00:00:00 2001 From: Jerry Johns Date: Thu, 7 Jul 2022 12:02:46 -0700 Subject: [PATCH 4/4] Fix build --- src/controller/python/chip/native/CommonStackInit.cpp | 9 +++++---- src/controller/python/chip/native/__init__.py | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/controller/python/chip/native/CommonStackInit.cpp b/src/controller/python/chip/native/CommonStackInit.cpp index a6a9b3e455899e..5fa470713b2079 100644 --- a/src/controller/python/chip/native/CommonStackInit.cpp +++ b/src/controller/python/chip/native/CommonStackInit.cpp @@ -31,6 +31,7 @@ #include #include +#include #include #include #include @@ -41,11 +42,11 @@ static_assert(std::is_same::value, "pyth extern "C" { -CHIP_ERROR pychip_CommonStackInit() +chip::ChipError::StorageType pychip_CommonStackInit() { - ReturnErrorOnFailure(chip::Platform::MemoryInit()); - ReturnErrorOnFailure(chip::DeviceLayer::PlatformMgr().InitChipStack()); - return CHIP_NO_ERROR; + ReturnErrorOnFailure(chip::Platform::MemoryInit().AsInteger()); + ReturnErrorOnFailure(chip::DeviceLayer::PlatformMgr().InitChipStack().AsInteger()); + return CHIP_NO_ERROR.AsInteger(); } void pychip_CommonStackShutdown() diff --git a/src/controller/python/chip/native/__init__.py b/src/controller/python/chip/native/__init__.py index e147f684f605e3..44b4ccd89f015e 100644 --- a/src/controller/python/chip/native/__init__.py +++ b/src/controller/python/chip/native/__init__.py @@ -76,7 +76,7 @@ def GetLibraryHandle() -> ctypes.CDLL: if _nativeLibraryHandle is None: _nativeLibraryHandle = ctypes.CDLL(FindNativeLibraryPath()) setter = NativeLibraryHandleMethodArguments(_nativeLibraryHandle) - setter.Set("pychip_CommonStackInit", None, []) + setter.Set("pychip_CommonStackInit", ctypes.c_uint32, []) # # We've a split initialization model with some init happening here and some other