From 7ee2771dc2bbd3da4ce9d66715b03d895ea4f37b Mon Sep 17 00:00:00 2001 From: Jerry Johns Date: Tue, 8 Jun 2021 22:00:49 -0700 Subject: [PATCH 1/3] Fix thread races in chip-tool Problem: This PR achieves the following to fix-up the various thread-races detected by tsan in chip-tool: Change: - Following the pattern of 'external synchronization', sprinkled LockChipStack() and UnlockChipStack() calls around key call sites that called into the stack from the various command logic in chip-tool - Removed usleep and global instance hacks. - Reverts changes in #7494 - Re-structured Command::Run to now have the bulk of the stack initialization and shutdown be managed before Run() is called in Commands::Run(), and an ExecutionContext object pointer be stashed inside the Command for convenient access. This reduces the changes of people writing new commands of getting stack initialization wrong. - Instead of sometimes using chip::Controller::DeviceController and sometimes DeviceCommissioner, just used the latter in all commands since that is the super-set class anyways. - Added a new 'StopEventLoopTask' that is thread-safe, that is needed to be called by application logic before DeviceController::Shutdown() can be called with external synchronization. - Pivots PlatformMgr::Shutdown() to not handle stopping the event queue, but only focus on cleaning up the stack objects. - Fixed up TestMdns as well along the way. Testing: - Enabled tsan using 'is_tsan' build arg and used that catch well over 10+ races, with not a single false-positive. - Ran through all the chip-tool command groups (pairing, IM, discover, testcluster, payload, etc) 10x each to ensure no regressions in functionality as well as ensuring clean shutdown with tsan. --- .../commands/clusters/ModelCommand.cpp | 38 +++--- .../commands/clusters/ModelCommand.h | 4 +- examples/chip-tool/commands/common/Command.h | 21 ++- .../chip-tool/commands/common/Commands.cpp | 57 ++++++-- examples/chip-tool/commands/common/Commands.h | 6 +- .../chip-tool/commands/discover/Commands.h | 4 +- .../commands/discover/DiscoverCommand.cpp | 34 +++-- .../commands/discover/DiscoverCommand.h | 6 +- .../commands/pairing/PairingCommand.cpp | 90 +++++++------ .../commands/pairing/PairingCommand.h | 3 +- .../payload/AdditionalDataParseCommand.cpp | 5 +- .../payload/AdditionalDataParseCommand.h | 2 +- .../payload/SetupPayloadParseCommand.cpp | 2 +- .../payload/SetupPayloadParseCommand.h | 2 +- .../commands/reporting/ReportingCommand.cpp | 44 +++---- .../commands/reporting/ReportingCommand.h | 5 +- .../chip-tool/commands/tests/TestCommand.cpp | 33 +++-- .../chip-tool/commands/tests/TestCommand.h | 7 +- .../templates/partials/test_cluster.zapt | 13 +- examples/minimal-mdns/client.cpp | 7 +- src/app/tests/integration/common.cpp | 1 + src/controller/CHIPDeviceController.cpp | 24 ++-- src/controller/CHIPDeviceController.h | 22 ++++ src/include/platform/PlatformManager.h | 58 ++++++++- .../GenericPlatformManagerImpl_FreeRTOS.cpp | 8 ++ .../GenericPlatformManagerImpl_FreeRTOS.h | 1 + .../GenericPlatformManagerImpl_POSIX.cpp | 123 +++++++++++++++++- .../GenericPlatformManagerImpl_POSIX.h | 18 +++ .../GenericPlatformManagerImpl_Zephyr.cpp | 8 ++ .../GenericPlatformManagerImpl_Zephyr.h | 1 + src/messaging/tests/echo/common.cpp | 1 + src/platform/Darwin/PlatformManagerImpl.h | 1 + src/platform/tests/TestMdns.cpp | 12 +- 33 files changed, 463 insertions(+), 198 deletions(-) diff --git a/examples/chip-tool/commands/clusters/ModelCommand.cpp b/examples/chip-tool/commands/clusters/ModelCommand.cpp index c556197ccea74a..55582dcca25892 100644 --- a/examples/chip-tool/commands/clusters/ModelCommand.cpp +++ b/examples/chip-tool/commands/clusters/ModelCommand.cpp @@ -37,39 +37,31 @@ void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aC "Default DispatchSingleClusterCommand is called, this should be replaced by actual dispatched for cluster commands"); } -CHIP_ERROR ModelCommand::Run(PersistentStorage & storage, NodeId localId, NodeId remoteId) +CHIP_ERROR ModelCommand::Run(NodeId localId, NodeId remoteId) { CHIP_ERROR err = CHIP_NO_ERROR; - chip::Controller::CommissionerInitParams initParams; - initParams.storageDelegate = &storage; - - err = mOpCredsIssuer.Initialize(storage); - VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Operational Cred Issuer: %s", ErrorStr(err))); - - initParams.operationalCredentialsDelegate = &mOpCredsIssuer; - - err = mCommissioner.SetUdpListenPort(storage.GetListenPort()); - VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Commissioner: %s", ErrorStr(err))); - - err = mCommissioner.Init(localId, initParams); - VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Commissioner: %s", ErrorStr(err))); + // + // Set this to true first BEFORE we send commands to ensure we don't + // end up in a situation where the response comes back faster than we can + // set the variable to true, which will cause it to block indefinitely. + // + UpdateWaitForResponse(true); + + { + chip::DeviceLayer::StackLock lock; - err = mCommissioner.ServiceEvents(); - VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Run Loop: %s", ErrorStr(err))); + err = GetExecContext()->commissioner->GetDevice(remoteId, &mDevice); + VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(chipTool, "Init failure! No pairing for device: %" PRIu64, localId)); - err = mCommissioner.GetDevice(remoteId, &mDevice); - VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(chipTool, "Init failure! No pairing for device: %" PRIu64, localId)); + err = SendCommand(mDevice, mEndPointId); + VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(chipTool, "Failed to send message: %s", ErrorStr(err))); + } - UpdateWaitForResponse(true); - err = SendCommand(mDevice, mEndPointId); - VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(chipTool, "Failed to send message: %s", ErrorStr(err))); WaitForResponse(kWaitDurationInSeconds); VerifyOrExit(GetCommandExitStatus(), err = CHIP_ERROR_INTERNAL); exit: - mCommissioner.ServiceEventSignal(); - mCommissioner.Shutdown(); return err; } diff --git a/examples/chip-tool/commands/clusters/ModelCommand.h b/examples/chip-tool/commands/clusters/ModelCommand.h index fefc9c9db1306d..f35f162468f430 100644 --- a/examples/chip-tool/commands/clusters/ModelCommand.h +++ b/examples/chip-tool/commands/clusters/ModelCommand.h @@ -36,13 +36,11 @@ class ModelCommand : public Command void AddArguments() { AddArgument("endpoint-id", CHIP_ZCL_ENDPOINT_MIN, CHIP_ZCL_ENDPOINT_MAX, &mEndPointId); } /////////// Command Interface ///////// - CHIP_ERROR Run(PersistentStorage & storage, NodeId localId, NodeId remoteId) override; + CHIP_ERROR Run(NodeId localId, NodeId remoteId) override; virtual CHIP_ERROR SendCommand(ChipDevice * device, uint8_t endPointId) = 0; private: - ChipDeviceController mCommissioner; ChipDevice * mDevice; - chip::Controller::ExampleOperationalCredentialsIssuer mOpCredsIssuer; uint8_t mEndPointId; }; diff --git a/examples/chip-tool/commands/common/Command.h b/examples/chip-tool/commands/common/Command.h index bcb09c30112267..c08dcbbef3cd74 100644 --- a/examples/chip-tool/commands/common/Command.h +++ b/examples/chip-tool/commands/common/Command.h @@ -18,6 +18,7 @@ #pragma once +#include "controller/ExampleOperationalCredentialsIssuer.h" #include #include #include @@ -90,9 +91,23 @@ class Command ::chip::Inet::InterfaceId interfaceId; }; + /** + * @brief + * Encapsulates key objects in the CHIP stack that need continued + * access, so wrapping it in here makes it nice and compactly encapsulated. + */ + struct ExecutionContext + { + ChipDeviceCommissioner * commissioner; + chip::Controller::ExampleOperationalCredentialsIssuer * opCredsIssuer; + PersistentStorage * storage; + }; + Command(const char * commandName) : mName(commandName) {} virtual ~Command() {} + void SetExecutionContext(ExecutionContext & execContext) { mExecContext = &execContext; } + const char * GetName(void) const { return mName; } const char * GetAttribute(void) const; const char * GetArgumentName(size_t index) const; @@ -147,7 +162,7 @@ class Command return AddArgument(name, min, max, reinterpret_cast(out), Number_uint64); } - virtual CHIP_ERROR Run(PersistentStorage & storage, NodeId localId, NodeId remoteId) = 0; + virtual CHIP_ERROR Run(NodeId localId, NodeId remoteId) = 0; bool GetCommandExitStatus() const { return mCommandExitStatus; } void SetCommandExitStatus(bool status) @@ -159,6 +174,10 @@ class Command void UpdateWaitForResponse(bool value); void WaitForResponse(uint16_t duration); +protected: + ExecutionContext * GetExecContext() { return mExecContext; } + ExecutionContext * mExecContext; + private: bool InitArgument(size_t argIndex, char * argValue); size_t AddArgument(const char * name, int64_t min, uint64_t max, void * out, ArgumentType type); diff --git a/examples/chip-tool/commands/common/Commands.cpp b/examples/chip-tool/commands/common/Commands.cpp index 604ded8655af05..5374adaacc40b0 100644 --- a/examples/chip-tool/commands/common/Commands.cpp +++ b/examples/chip-tool/commands/common/Commands.cpp @@ -41,7 +41,7 @@ void Commands::Register(const char * clusterName, commands_list commandsList) int Commands::Run(NodeId localId, NodeId remoteId, int argc, char ** argv) { CHIP_ERROR err = CHIP_NO_ERROR; - PersistentStorage storage; + chip::Controller::CommissionerInitParams initParams; err = chip::Platform::MemoryInit(); VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init Memory failure: %s", chip::ErrorStr(err))); @@ -51,19 +51,48 @@ int Commands::Run(NodeId localId, NodeId remoteId, int argc, char ** argv) SuccessOrExit(err = chip::DeviceLayer::Internal::BLEMgrImpl().ConfigureBle(/* BLE adapter ID */ 0, /* BLE central */ true)); #endif - err = storage.Init(); + err = mStorage.Init(); VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init Storage failure: %s", chip::ErrorStr(err))); - chip::Logging::SetLogFilter(storage.GetLoggingLevel()); + chip::Logging::SetLogFilter(mStorage.GetLoggingLevel()); - err = RunCommand(storage, localId, remoteId, argc, argv); + initParams.storageDelegate = &mStorage; + + err = mOpCredsIssuer.Initialize(mStorage); + VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Operational Cred Issuer: %s", chip::ErrorStr(err))); + + initParams.operationalCredentialsDelegate = &mOpCredsIssuer; + + err = mController.SetUdpListenPort(mStorage.GetListenPort()); + VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Commissioner: %s", chip::ErrorStr(err))); + + err = mController.Init(localId, initParams); + VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Commissioner: %s", chip::ErrorStr(err))); + + err = mController.ServiceEvents(); + VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Run Loop: %s", chip::ErrorStr(err))); + + err = RunCommand(localId, remoteId, argc, argv); SuccessOrExit(err); exit: +#if CONFIG_DEVICE_LAYER + chip::DeviceLayer::PlatformMgr().StopEventLoopTask(); +#endif + + // + // We can call DeviceController::Shutdown() safely without grabbing the stack lock + // since the CHIP thread and event queue have been stopped, preventing any thread + // races. + // + // TODO: This doesn't hold true on Darwin, issue #7557 tracks the problem. + // + mController.Shutdown(); + return (err == CHIP_NO_ERROR) ? EXIT_SUCCESS : EXIT_FAILURE; } -CHIP_ERROR Commands::RunCommand(PersistentStorage & storage, NodeId localId, NodeId remoteId, int argc, char ** argv) +CHIP_ERROR Commands::RunCommand(NodeId localId, NodeId remoteId, int argc, char ** argv) { CHIP_ERROR err = CHIP_NO_ERROR; std::map::iterator cluster; @@ -125,11 +154,21 @@ CHIP_ERROR Commands::RunCommand(PersistentStorage & storage, NodeId localId, Nod ExitNow(err = CHIP_ERROR_INVALID_ARGUMENT); } - err = command->Run(storage, localId, remoteId); - if (err != CHIP_NO_ERROR) { - ChipLogError(chipTool, "Run command failure: %s", chip::ErrorStr(err)); - ExitNow(); + Command::ExecutionContext execContext; + + execContext.commissioner = &mController; + execContext.opCredsIssuer = &mOpCredsIssuer; + execContext.storage = &mStorage; + + command->SetExecutionContext(execContext); + + err = command->Run(localId, remoteId); + if (err != CHIP_NO_ERROR) + { + ChipLogError(chipTool, "Run command failure: %s", chip::ErrorStr(err)); + ExitNow(); + } } exit: diff --git a/examples/chip-tool/commands/common/Commands.h b/examples/chip-tool/commands/common/Commands.h index 68dcb5a6d5aefb..10f1a77891cee0 100644 --- a/examples/chip-tool/commands/common/Commands.h +++ b/examples/chip-tool/commands/common/Commands.h @@ -20,6 +20,7 @@ #include "../../config/PersistentStorage.h" #include "Command.h" +#include #include class Commands @@ -32,7 +33,7 @@ class Commands int Run(NodeId localId, NodeId remoteId, int argc, char ** argv); private: - CHIP_ERROR RunCommand(PersistentStorage & storage, NodeId localId, NodeId remoteId, int argc, char ** argv); + CHIP_ERROR RunCommand(NodeId localId, NodeId remoteId, int argc, char ** argv); std::map::iterator GetCluster(std::string clusterName); Command * GetCommand(CommandsVector & commands, std::string commandName); Command * GetGlobalCommand(CommandsVector & commands, std::string commandName, std::string attributeName); @@ -44,4 +45,7 @@ class Commands void ShowCommand(std::string executable, std::string clusterName, Command * command); std::map mClusters; + chip::Controller::DeviceCommissioner mController; + chip::Controller::ExampleOperationalCredentialsIssuer mOpCredsIssuer; + PersistentStorage mStorage; }; diff --git a/examples/chip-tool/commands/discover/Commands.h b/examples/chip-tool/commands/discover/Commands.h index 91efeaf0471cbb..96737eb3d61aa1 100644 --- a/examples/chip-tool/commands/discover/Commands.h +++ b/examples/chip-tool/commands/discover/Commands.h @@ -66,9 +66,9 @@ class Update : public DiscoverCommand CHIP_ERROR RunCommand(NodeId remoteId, uint64_t fabricId) override { ChipDevice * device; - ReturnErrorOnFailure(mCommissioner.GetDevice(remoteId, &device)); + ReturnErrorOnFailure(GetExecContext()->commissioner->GetDevice(remoteId, &device)); ChipLogProgress(chipTool, "Mdns: Updating NodeId: %" PRIx64 " FabricId: %" PRIx64 " ...", remoteId, fabricId); - return mCommissioner.UpdateDevice(device, fabricId); + return GetExecContext()->commissioner->UpdateDevice(device, fabricId); } /////////// DeviceAddressUpdateDelegate Interface ///////// diff --git a/examples/chip-tool/commands/discover/DiscoverCommand.cpp b/examples/chip-tool/commands/discover/DiscoverCommand.cpp index e7db1bdd00c3da..506b460b16c856 100644 --- a/examples/chip-tool/commands/discover/DiscoverCommand.cpp +++ b/examples/chip-tool/commands/discover/DiscoverCommand.cpp @@ -20,27 +20,33 @@ constexpr uint16_t kWaitDurationInSeconds = 30; -CHIP_ERROR DiscoverCommand::Run(PersistentStorage & storage, NodeId localId, NodeId remoteId) +CHIP_ERROR DiscoverCommand::Run(NodeId localId, NodeId remoteId) { - chip::Controller::CommissionerInitParams params; + CHIP_ERROR err; - params.storageDelegate = &storage; - params.mDeviceAddressUpdateDelegate = this; - params.operationalCredentialsDelegate = &mOpCredsIssuer; - - ReturnErrorOnFailure(mCommissioner.SetUdpListenPort(storage.GetListenPort())); - ReturnErrorOnFailure(mCommissioner.Init(localId, params)); - ReturnErrorOnFailure(mCommissioner.ServiceEvents()); + // + // Set this to true first BEFORE we send commands to ensure we don't + // end up in a situation where the response comes back faster than we can + // set the variable to true, which will cause it to block indefinitely. + // + UpdateWaitForResponse(true); + + { + chip::DeviceLayer::StackLock lock; - ReturnErrorOnFailure(RunCommand(mNodeId, mFabricId)); + GetExecContext()->commissioner->RegisterDeviceAddressUpdateDelegate(this); + err = RunCommand(mNodeId, mFabricId); + SuccessOrExit(err); + } - UpdateWaitForResponse(true); WaitForResponse(kWaitDurationInSeconds); - mCommissioner.ServiceEventSignal(); - mCommissioner.Shutdown(); +exit: + if (err != CHIP_NO_ERROR) + { + return err; + } VerifyOrReturnError(GetCommandExitStatus(), CHIP_ERROR_INTERNAL); - return CHIP_NO_ERROR; } diff --git a/examples/chip-tool/commands/discover/DiscoverCommand.h b/examples/chip-tool/commands/discover/DiscoverCommand.h index 6743fab8f93cdc..801e43b82bb15b 100644 --- a/examples/chip-tool/commands/discover/DiscoverCommand.h +++ b/examples/chip-tool/commands/discover/DiscoverCommand.h @@ -35,15 +35,11 @@ class DiscoverCommand : public Command, public chip::Controller::DeviceAddressUp void OnAddressUpdateComplete(NodeId nodeId, CHIP_ERROR error) override{}; /////////// Command Interface ///////// - CHIP_ERROR Run(PersistentStorage & storage, NodeId localId, NodeId remoteId) override; + CHIP_ERROR Run(NodeId localId, NodeId remoteId) override; virtual CHIP_ERROR RunCommand(NodeId remoteId, uint64_t fabricId) = 0; -protected: - ChipDeviceCommissioner mCommissioner; - private: chip::NodeId mNodeId; uint64_t mFabricId; - chip::Controller::ExampleOperationalCredentialsIssuer mOpCredsIssuer; }; diff --git a/examples/chip-tool/commands/pairing/PairingCommand.cpp b/examples/chip-tool/commands/pairing/PairingCommand.cpp index 1a510e3a0caf42..e96897106c608d 100644 --- a/examples/chip-tool/commands/pairing/PairingCommand.cpp +++ b/examples/chip-tool/commands/pairing/PairingCommand.cpp @@ -17,6 +17,7 @@ */ #include "PairingCommand.h" +#include "platform/PlatformManager.h" #include using namespace ::chip; @@ -26,35 +27,21 @@ constexpr uint64_t kBreadcrumb = 0; constexpr uint32_t kTimeoutMs = 6000; constexpr uint8_t kTemporaryThreadNetworkId[] = { 0x01, 0x23, 0x45, 0x67, 0x89, 0xab, 0xcd, 0xef }; -CHIP_ERROR PairingCommand::Run(PersistentStorage & storage, NodeId localId, NodeId remoteId) +CHIP_ERROR PairingCommand::Run(NodeId localId, NodeId remoteId) { CHIP_ERROR err = CHIP_NO_ERROR; - chip::Controller::CommissionerInitParams params; - params.storageDelegate = &storage; - params.mDeviceAddressUpdateDelegate = this; - params.pairingDelegate = this; - - err = mOpCredsIssuer.Initialize(storage); - VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Operational Cred Issuer: %s", ErrorStr(err))); - - params.operationalCredentialsDelegate = &mOpCredsIssuer; - - err = mCommissioner.SetUdpListenPort(storage.GetListenPort()); - VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Commissioner: %s", ErrorStr(err))); - - err = mCommissioner.Init(localId, params); - VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Commissioner: %s", ErrorStr(err))); + { + chip::DeviceLayer::StackLock lock; - err = mCommissioner.ServiceEvents(); - VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Run Loop: %s", ErrorStr(err))); + GetExecContext()->commissioner->RegisterDeviceAddressUpdateDelegate(this); + GetExecContext()->commissioner->RegisterPairingDelegate(this); + } err = RunInternal(remoteId); VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(chipTool, "Init Failure! PairDevice: %s", ErrorStr(err))); exit: - mCommissioner.ServiceEventSignal(); - mCommissioner.Shutdown(); if (err == CHIP_NO_ERROR) { @@ -71,26 +58,43 @@ CHIP_ERROR PairingCommand::RunInternal(NodeId remoteId) mRemoteId = remoteId; InitCallbacks(); + + // + // Set this to true first BEFORE we send commands to ensure we don't + // end up in a situation where the response comes back faster than we can + // set the variable to true, which will cause it to block indefinitely. + // UpdateWaitForResponse(true); - switch (mPairingMode) + + // + // We're about to call methods into the stack, so lock + // appropriately. None of the following calls below before the unlock are, nor should be, + // blocking. + // { - case PairingMode::None: - err = Unpair(remoteId); - break; - case PairingMode::Bypass: - err = PairWithoutSecurity(remoteId, PeerAddress::UDP(mRemoteAddr.address, mRemotePort)); - break; - case PairingMode::Ble: - err = Pair(remoteId, PeerAddress::BLE()); - break; - case PairingMode::OnNetwork: - case PairingMode::SoftAP: - err = Pair(remoteId, PeerAddress::UDP(mRemoteAddr.address, mRemotePort)); - break; - case PairingMode::Ethernet: - err = Pair(remoteId, PeerAddress::UDP(mRemoteAddr.address, mRemotePort)); - break; + chip::DeviceLayer::StackLock lock; + + switch (mPairingMode) + { + case PairingMode::None: + err = Unpair(remoteId); + break; + case PairingMode::Bypass: + err = PairWithoutSecurity(remoteId, PeerAddress::UDP(mRemoteAddr.address, mRemotePort)); + break; + case PairingMode::Ble: + err = Pair(remoteId, PeerAddress::BLE()); + break; + case PairingMode::OnNetwork: + case PairingMode::SoftAP: + err = Pair(remoteId, PeerAddress::UDP(mRemoteAddr.address, mRemotePort)); + break; + case PairingMode::Ethernet: + err = Pair(remoteId, PeerAddress::UDP(mRemoteAddr.address, mRemotePort)); + break; + } } + WaitForResponse(kWaitDurationInSeconds); ReleaseCallbacks(); @@ -102,19 +106,19 @@ CHIP_ERROR PairingCommand::Pair(NodeId remoteId, PeerAddress address) RendezvousParameters params = RendezvousParameters().SetSetupPINCode(mSetupPINCode).SetDiscriminator(mDiscriminator).SetPeerAddress(address); - return mCommissioner.PairDevice(remoteId, params); + return GetExecContext()->commissioner->PairDevice(remoteId, params); } CHIP_ERROR PairingCommand::PairWithoutSecurity(NodeId remoteId, PeerAddress address) { ChipSerializedDevice serializedTestDevice; - return mCommissioner.PairTestDeviceWithoutSecurity(remoteId, address, serializedTestDevice); + return GetExecContext()->commissioner->PairTestDeviceWithoutSecurity(remoteId, address, serializedTestDevice); } CHIP_ERROR PairingCommand::Unpair(NodeId remoteId) { SetCommandExitStatus(true); - return mCommissioner.UnpairDevice(remoteId); + return GetExecContext()->commissioner->UnpairDevice(remoteId); } void PairingCommand::OnStatusUpdate(DevicePairingDelegate::Status status) @@ -172,7 +176,7 @@ CHIP_ERROR PairingCommand::SetupNetwork() case PairingNetworkType::WiFi: case PairingNetworkType::Ethernet: case PairingNetworkType::Thread: - err = mCommissioner.GetDevice(mRemoteId, &mDevice); + err = GetExecContext()->commissioner->GetDevice(mRemoteId, &mDevice); VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(chipTool, "Setup failure! No pairing for device: %" PRIu64, mRemoteId)); mCluster.Associate(mDevice, mEndpointId); @@ -312,9 +316,9 @@ void PairingCommand::OnEnableNetworkResponse(void * context, uint8_t errorCode, CHIP_ERROR PairingCommand::UpdateNetworkAddress() { - ReturnErrorOnFailure(mCommissioner.GetDevice(mRemoteId, &mDevice)); + ReturnErrorOnFailure(GetExecContext()->commissioner->GetDevice(mRemoteId, &mDevice)); ChipLogProgress(chipTool, "Mdns: Updating NodeId: %" PRIx64 " FabricId: %" PRIx64 " ...", mRemoteId, mFabricId); - return mCommissioner.UpdateDevice(mDevice, mFabricId); + return GetExecContext()->commissioner->UpdateDevice(mDevice, mFabricId); } void PairingCommand::OnAddressUpdateComplete(NodeId nodeId, CHIP_ERROR err) diff --git a/examples/chip-tool/commands/pairing/PairingCommand.h b/examples/chip-tool/commands/pairing/PairingCommand.h index 2d0b7abbbf1572..7fdc1375fb45d9 100644 --- a/examples/chip-tool/commands/pairing/PairingCommand.h +++ b/examples/chip-tool/commands/pairing/PairingCommand.h @@ -97,7 +97,7 @@ class PairingCommand : public Command, } /////////// Command Interface ///////// - CHIP_ERROR Run(PersistentStorage & storage, NodeId localId, NodeId remoteId) override; + CHIP_ERROR Run(NodeId localId, NodeId remoteId) override; /////////// DevicePairingDelegate Interface ///////// void OnStatusUpdate(chip::Controller::DevicePairingDelegate::Status status) override; @@ -143,7 +143,6 @@ class PairingCommand : public Command, chip::Callback::Callback * mOnAddWiFiNetworkCallback; chip::Callback::Callback * mOnEnableNetworkCallback; chip::Callback::Callback * mOnFailureCallback; - ChipDeviceCommissioner mCommissioner; ChipDevice * mDevice; chip::Controller::NetworkCommissioningCluster mCluster; chip::EndpointId mEndpointId = 0; diff --git a/examples/chip-tool/commands/payload/AdditionalDataParseCommand.cpp b/examples/chip-tool/commands/payload/AdditionalDataParseCommand.cpp index 917b21c0ff1990..4855350b6b6d1a 100644 --- a/examples/chip-tool/commands/payload/AdditionalDataParseCommand.cpp +++ b/examples/chip-tool/commands/payload/AdditionalDataParseCommand.cpp @@ -24,7 +24,7 @@ using namespace ::chip; using namespace ::chip::SetupPayloadData; -CHIP_ERROR AdditionalDataParseCommand::Run(PersistentStorage & storage, NodeId localId, NodeId remoteId) +CHIP_ERROR AdditionalDataParseCommand::Run(NodeId localId, NodeId remoteId) { std::vector payloadData; AdditionalDataPayload resultPayload; @@ -40,9 +40,12 @@ CHIP_ERROR AdditionalDataParseCommand::Run(PersistentStorage & storage, NodeId l uint8_t x = (uint8_t) stoi(str, 0, 16); payloadData.push_back(x); } + err = AdditionalDataPayloadParser(payloadData.data(), (uint32_t) payloadData.size()).populatePayload(resultPayload); SuccessOrExit(err); + ChipLogProgress(chipTool, "AdditionalDataParseCommand, RotatingDeviceId=%s", resultPayload.rotatingDeviceId.c_str()); + exit: return err; } diff --git a/examples/chip-tool/commands/payload/AdditionalDataParseCommand.h b/examples/chip-tool/commands/payload/AdditionalDataParseCommand.h index ddb0bd462618d4..d3b38586ccee4d 100644 --- a/examples/chip-tool/commands/payload/AdditionalDataParseCommand.h +++ b/examples/chip-tool/commands/payload/AdditionalDataParseCommand.h @@ -24,7 +24,7 @@ class AdditionalDataParseCommand : public Command { public: AdditionalDataParseCommand() : Command("parse-additional-data-payload") { AddArgument("payload", &mPayload); } - CHIP_ERROR Run(PersistentStorage & storage, NodeId localId, NodeId remoteId) override; + CHIP_ERROR Run(NodeId localId, NodeId remoteId) override; private: char * mPayload; diff --git a/examples/chip-tool/commands/payload/SetupPayloadParseCommand.cpp b/examples/chip-tool/commands/payload/SetupPayloadParseCommand.cpp index 3ad82f3c38ae4c..2f6f4e8c8ef9bd 100644 --- a/examples/chip-tool/commands/payload/SetupPayloadParseCommand.cpp +++ b/examples/chip-tool/commands/payload/SetupPayloadParseCommand.cpp @@ -23,7 +23,7 @@ using namespace ::chip; -CHIP_ERROR SetupPayloadParseCommand::Run(PersistentStorage & storage, NodeId localId, NodeId remoteId) +CHIP_ERROR SetupPayloadParseCommand::Run(NodeId localId, NodeId remoteId) { std::string codeString(mCode); SetupPayload payload; diff --git a/examples/chip-tool/commands/payload/SetupPayloadParseCommand.h b/examples/chip-tool/commands/payload/SetupPayloadParseCommand.h index 2e09ee743109b9..cf7af18e65e75a 100644 --- a/examples/chip-tool/commands/payload/SetupPayloadParseCommand.h +++ b/examples/chip-tool/commands/payload/SetupPayloadParseCommand.h @@ -25,7 +25,7 @@ class SetupPayloadParseCommand : public Command { public: SetupPayloadParseCommand() : Command("parse-setup-payload") { AddArgument("payload", &mCode); } - CHIP_ERROR Run(PersistentStorage & storage, NodeId localId, NodeId remoteId) override; + CHIP_ERROR Run(NodeId localId, NodeId remoteId) override; private: char * mCode; diff --git a/examples/chip-tool/commands/reporting/ReportingCommand.cpp b/examples/chip-tool/commands/reporting/ReportingCommand.cpp index 447c1c82eb9c7c..4612301e8d07b4 100644 --- a/examples/chip-tool/commands/reporting/ReportingCommand.cpp +++ b/examples/chip-tool/commands/reporting/ReportingCommand.cpp @@ -28,43 +28,33 @@ namespace { constexpr uint16_t kWaitDurationInSeconds = UINT16_MAX; } // namespace -CHIP_ERROR ReportingCommand::Run(PersistentStorage & storage, NodeId localId, NodeId remoteId) +CHIP_ERROR ReportingCommand::Run(NodeId localId, NodeId remoteId) { CHIP_ERROR err = CHIP_NO_ERROR; - chip::Controller::BasicCluster cluster; - chip::Controller::CommissionerInitParams initParams; - - initParams.storageDelegate = &storage; - - err = mOpCredsIssuer.Initialize(storage); - VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Operational Cred Issuer: %s", ErrorStr(err))); - - initParams.operationalCredentialsDelegate = &mOpCredsIssuer; - err = mCommissioner.SetUdpListenPort(storage.GetListenPort()); - VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Commissioner: %s", ErrorStr(err))); - - err = mCommissioner.Init(localId, initParams); - VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Commissioner: %s", ErrorStr(err))); - - err = mCommissioner.ServiceEvents(); - VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Run Loop: %s", ErrorStr(err))); + // + // Set this to true first BEFORE we send commands to ensure we don't + // end up in a situation where the response comes back faster than we can + // set the variable to true, which will cause it to block indefinitely. + // + UpdateWaitForResponse(true); + + { + chip::DeviceLayer::StackLock lock; - err = mCommissioner.GetDevice(remoteId, &mDevice); - VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(chipTool, "Init failure! No pairing for device: %" PRIu64, localId)); + err = GetExecContext()->commissioner->GetDevice(remoteId, &mDevice); + VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(chipTool, "Init failure! No pairing for device: %" PRIu64, localId)); - AddReportCallbacks(mEndPointId); + AddReportCallbacks(mEndPointId); + cluster.Associate(mDevice, mEndPointId); - cluster.Associate(mDevice, mEndPointId); - err = cluster.MfgSpecificPing(nullptr, nullptr); - VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Ping failure: %s", ErrorStr(err))); + err = cluster.MfgSpecificPing(nullptr, nullptr); + VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Ping failure: %s", ErrorStr(err))); + } - UpdateWaitForResponse(true); WaitForResponse(kWaitDurationInSeconds); exit: - mCommissioner.ServiceEventSignal(); - mCommissioner.Shutdown(); return err; } diff --git a/examples/chip-tool/commands/reporting/ReportingCommand.h b/examples/chip-tool/commands/reporting/ReportingCommand.h index ff29239916a97f..59fd902b823fd3 100644 --- a/examples/chip-tool/commands/reporting/ReportingCommand.h +++ b/examples/chip-tool/commands/reporting/ReportingCommand.h @@ -36,14 +36,11 @@ class ReportingCommand : public Command } /////////// Command Interface ///////// - CHIP_ERROR Run(PersistentStorage & storage, NodeId localId, NodeId remoteId) override; + CHIP_ERROR Run(NodeId localId, NodeId remoteId) override; virtual void AddReportCallbacks(uint8_t endPointId) = 0; private: uint8_t mEndPointId; - - ChipDeviceCommissioner mCommissioner; ChipDevice * mDevice; - chip::Controller::ExampleOperationalCredentialsIssuer mOpCredsIssuer; }; diff --git a/examples/chip-tool/commands/tests/TestCommand.cpp b/examples/chip-tool/commands/tests/TestCommand.cpp index ff50df36116db6..e7f09ffc45ebcc 100644 --- a/examples/chip-tool/commands/tests/TestCommand.cpp +++ b/examples/chip-tool/commands/tests/TestCommand.cpp @@ -20,30 +20,29 @@ constexpr uint16_t kWaitDurationInSeconds = 30; -CHIP_ERROR TestCommand::Run(PersistentStorage & storage, NodeId localId, NodeId remoteId) +CHIP_ERROR TestCommand::Run(NodeId localId, NodeId remoteId) { - ReturnErrorOnFailure(mOpCredsIssuer.Initialize(storage)); + CHIP_ERROR err = CHIP_NO_ERROR; - chip::Controller::CommissionerInitParams params; - - params.storageDelegate = &storage; - params.operationalCredentialsDelegate = &mOpCredsIssuer; + // + // Set this to true first BEFORE we send commands to ensure we don't + // end up in a situation where the response comes back faster than we can + // set the variable to true, which will cause it to block indefinitely. + // + UpdateWaitForResponse(true); + + { + chip::DeviceLayer::StackLock lock; - ReturnErrorOnFailure(mCommissioner.SetUdpListenPort(storage.GetListenPort())); - ReturnErrorOnFailure(mCommissioner.Init(localId, params)); - ReturnErrorOnFailure(mCommissioner.ServiceEvents()); - ReturnErrorOnFailure(mCommissioner.GetDevice(remoteId, &mDevice)); + err = GetExecContext()->commissioner->GetDevice(remoteId, &mDevice); + ReturnErrorOnFailure(err); - ReturnErrorOnFailure(NextTest()); + err = NextTest(); + ReturnErrorOnFailure(err); + } - UpdateWaitForResponse(true); WaitForResponse(kWaitDurationInSeconds); - mCommissioner.ServiceEventSignal(); - - mCommissioner.Shutdown(); - VerifyOrReturnError(GetCommandExitStatus(), CHIP_ERROR_INTERNAL); - return CHIP_NO_ERROR; } diff --git a/examples/chip-tool/commands/tests/TestCommand.h b/examples/chip-tool/commands/tests/TestCommand.h index 574b9e609cecd6..16ae7c0f7583aa 100644 --- a/examples/chip-tool/commands/tests/TestCommand.h +++ b/examples/chip-tool/commands/tests/TestCommand.h @@ -18,7 +18,6 @@ #pragma once -#include "../../config/PersistentStorage.h" #include "../common/Command.h" #include @@ -28,14 +27,10 @@ class TestCommand : public Command TestCommand(const char * commandName) : Command(commandName) {} /////////// Command Interface ///////// - CHIP_ERROR Run(PersistentStorage & storage, NodeId localId, NodeId remoteId) override; + CHIP_ERROR Run(NodeId localId, NodeId remoteId) override; virtual CHIP_ERROR NextTest() = 0; protected: - ChipDeviceCommissioner mCommissioner; ChipDevice * mDevice; - -private: - chip::Controller::ExampleOperationalCredentialsIssuer mOpCredsIssuer; }; diff --git a/examples/chip-tool/templates/partials/test_cluster.zapt b/examples/chip-tool/templates/partials/test_cluster.zapt index 5d461c438c043a..2a201787bb4525 100644 --- a/examples/chip-tool/templates/partials/test_cluster.zapt +++ b/examples/chip-tool/templates/partials/test_cluster.zapt @@ -2,7 +2,7 @@ class {{asCamelCased filename false}}: public TestCommand { public: - {{asCamelCased filename false}}(): TestCommand("{{filename}}"), mTestIndex(0) {} + {{asCamelCased filename false}}(): TestCommand("{{filename}}") {} /////////// TestCommand Interface ///////// CHIP_ERROR NextTest() override @@ -15,11 +15,7 @@ class {{asCamelCased filename false}}: public TestCommand SetCommandExitStatus(true); } - // Ensure we increment mTestIndex before we start running the relevant - // command. That way if we lose the timeslice after we send the message - // but before our function call returns, we won't end up with an - // incorrect mTestIndex value observed when we get the response. - switch (mTestIndex++) + switch (mTestIndex) { {{#chip_tests_items}} case {{index}}: @@ -27,6 +23,7 @@ class {{asCamelCased filename false}}: public TestCommand break; {{/chip_tests_items}} } + mTestIndex++; if (CHIP_NO_ERROR != err) { @@ -39,8 +36,8 @@ class {{asCamelCased filename false}}: public TestCommand private: - std::atomic_uint16_t mTestIndex; - const uint16_t mTestCount = {{totalTests}}; + uint16_t mTestIndex = 0; + uint16_t mTestCount = {{totalTests}}; // // Tests methods diff --git a/examples/minimal-mdns/client.cpp b/examples/minimal-mdns/client.cpp index ddfea2b56c13a4..ddc1abadd5821b 100644 --- a/examples/minimal-mdns/client.cpp +++ b/examples/minimal-mdns/client.cpp @@ -332,7 +332,12 @@ int main(int argc, char ** args) if (DeviceLayer::SystemLayer.NewTimer(timer) == CHIP_NO_ERROR) { timer->Start( - gOptions.runtimeMs, [](System::Layer *, void *, System::Error err) { DeviceLayer::PlatformMgr().Shutdown(); }, nullptr); + gOptions.runtimeMs, + [](System::Layer *, void *, System::Error err) { + DeviceLayer::PlatformMgr().StopEventLoopTask(); + DeviceLayer::PlatformMgr().Shutdown(); + }, + nullptr); } else { diff --git a/src/app/tests/integration/common.cpp b/src/app/tests/integration/common.cpp index be0cbcd6ec9b3d..ba1398062879b5 100644 --- a/src/app/tests/integration/common.cpp +++ b/src/app/tests/integration/common.cpp @@ -58,6 +58,7 @@ void InitializeChip(void) void ShutdownChip(void) { + chip::DeviceLayer::PlatformMgr().StopEventLoopTask(); chip::DeviceLayer::PlatformMgr().Shutdown(); gMessageCounterManager.Shutdown(); gExchangeManager.Shutdown(); diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index af1b870fbe00e6..c95f41fc148193 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -207,13 +207,11 @@ CHIP_ERROR DeviceController::Init(NodeId localDeviceId, ControllerInitParams par mExchangeMgr->SetDelegate(this); #if CHIP_DEVICE_CONFIG_ENABLE_MDNS - if (params.mDeviceAddressUpdateDelegate != nullptr) - { - err = Mdns::Resolver::Instance().SetResolverDelegate(this); - SuccessOrExit(err); + err = Mdns::Resolver::Instance().SetResolverDelegate(this); + SuccessOrExit(err); + + RegisterDeviceAddressUpdateDelegate(params.mDeviceAddressUpdateDelegate); - mDeviceAddressUpdateDelegate = params.mDeviceAddressUpdateDelegate; - } Mdns::Resolver::Instance().StartResolver(mInetLayer, kMdnsPort); #endif // CHIP_DEVICE_CONFIG_ENABLE_MDNS @@ -293,11 +291,15 @@ CHIP_ERROR DeviceController::Shutdown() ChipLogDetail(Controller, "Shutting down the controller"); #if CONFIG_DEVICE_LAYER - // Start by shutting down the PlatformManager. This will ensure, with - // reasonable synchronization, that we stop processing of incoming messages - // before doing any other shutdown work. Otherwise we can end up trying to - // process incoming messages in a partially shut down state, which is not - // great at all. + // + // We can safely call PlatformMgr().Shutdown(), which like DeviceController::Shutdown(), + // expects to be called with external thread synchronization and will not try to acquire the + // stack lock. + // + // Actually stopping the event queue is a separable call that applications will have to sequence. + // Consumers are expected to call PlaformMgr().StopEventLoopTask() before calling + // DeviceController::Shutdown() in the CONFIG_DEVICE_LAYER configuration + // ReturnErrorOnFailure(DeviceLayer::PlatformMgr().Shutdown()); #else mInetLayer->Shutdown(); diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 47f17188e1021e..ae29fbc4ed42fa 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -190,6 +190,15 @@ class DLL_EXPORT DeviceController : public Messaging::ExchangeDelegate, CHIP_ERROR Init(NodeId localDeviceId, ControllerInitParams params); + /** + * @brief + * Tears down the entirety of the stack, including destructing key objects in the system. + * This expects to be called with external thread synchronization, and will not internally + * grab the CHIP stack lock. + * + * This will also not stop the CHIP event queue / thread (if one exists). Consumers are expected to + * ensure this happend before calling this method. + */ virtual CHIP_ERROR Shutdown(); /** @@ -237,6 +246,10 @@ class DLL_EXPORT DeviceController : public Messaging::ExchangeDelegate, virtual void ReleaseDevice(Device * device); +#if CHIP_DEVICE_CONFIG_ENABLE_MDNS + void RegisterDeviceAddressUpdateDelegate(DeviceAddressUpdateDelegate * delegate) { mDeviceAddressUpdateDelegate = delegate; } +#endif + // ----- IO ----- /** * @brief @@ -374,6 +387,13 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, public SessionEst */ CHIP_ERROR Init(NodeId localDeviceId, CommissionerInitParams params); + /** + * @brief + * Tears down the entirety of the stack, including destructing key objects in the system. + * This is not a thread-safe API, and should be called with external synchronization. + * + * Please see implementation for more details. + */ CHIP_ERROR Shutdown() override; // ----- Connection Management ----- @@ -471,6 +491,8 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, public SessionEst #endif + void RegisterPairingDelegate(DevicePairingDelegate * pairingDelegate) { mPairingDelegate = pairingDelegate; } + private: DevicePairingDelegate * mPairingDelegate; diff --git a/src/include/platform/PlatformManager.h b/src/include/platform/PlatformManager.h index 041f84d8457129..5364e007d49f36 100644 --- a/src/include/platform/PlatformManager.h +++ b/src/include/platform/PlatformManager.h @@ -91,6 +91,7 @@ class PlatformManager void ScheduleWork(AsyncWorkFunct workFunct, intptr_t arg = 0); void RunEventLoop(); CHIP_ERROR StartEventLoopTask(); + CHIP_ERROR StopEventLoopTask(); void LockChipStack(); bool TryLockChipStack(); void UnlockChipStack(); @@ -171,6 +172,19 @@ extern PlatformManager & PlatformMgr(); */ extern PlatformManagerImpl & PlatformMgrImpl(); +/** + * @brief + * RAII locking for PlatformManager to simplify management of + * LockChipStack()/UnlockChipStack calls. + */ +class StackLock +{ +public: + StackLock() { PlatformMgr().LockChipStack(); } + + ~StackLock() { PlatformMgr().UnlockChipStack(); } +}; + } // namespace DeviceLayer } // namespace chip @@ -233,11 +247,49 @@ inline void PlatformManager::RunEventLoop() static_cast(this)->_RunEventLoop(); } +/** + * @brief + * Starts the stack on its own task with an associated event queue + * to dispatch and handle events posted to that task. + * + * This is thread-safe. + * This is *NOT SAFE* to call from within the CHIP event loop since it can grab the stack lock. + */ inline CHIP_ERROR PlatformManager::StartEventLoopTask() { return static_cast(this)->_StartEventLoopTask(); } +/** + * @brief + * This will trigger the event loop to exit and block till it has exited the loop. + * This prevents the processing of any further events in the queue. + * + * Additionally, this stops the CHIP task if the following criteria are met: + * 1. One was created earlier through a call to StartEventLoopTask + * 2. This call isn't being made from that task. + * + * This is safe to call from any task. + * This is safe to call from within the CHIP event loop. + * + */ +inline CHIP_ERROR PlatformManager::StopEventLoopTask() +{ + return static_cast(this)->_StopEventLoopTask(); +} + +/** + * @brief + * Shuts down and cleans up the main objects in the CHIP stack. + * This DOES NOT stop the chip thread or event queue from running. + * + */ +inline CHIP_ERROR PlatformManager::Shutdown() +{ + mInitialized = false; + return static_cast(this)->_Shutdown(); +} + inline void PlatformManager::LockChipStack() { static_cast(this)->_LockChipStack(); @@ -268,11 +320,5 @@ inline CHIP_ERROR PlatformManager::StartChipTimer(uint32_t durationMS) return static_cast(this)->_StartChipTimer(durationMS); } -inline CHIP_ERROR PlatformManager::Shutdown() -{ - mInitialized = false; - return static_cast(this)->_Shutdown(); -} - } // namespace DeviceLayer } // namespace chip diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp b/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp index dae75bd44fa6a1..0bd42edac3980b 100644 --- a/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp +++ b/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp @@ -241,6 +241,14 @@ void GenericPlatformManagerImpl_FreeRTOS::PostEventFromISR(const Chip template CHIP_ERROR GenericPlatformManagerImpl_FreeRTOS::_Shutdown(void) { + VerifyOrDieWithMsg(false, DeviceLayer, "Shutdown is not implemented"); + return CHIP_ERROR_NOT_IMPLEMENTED; +} + +template +CHIP_ERROR GenericPlatformManagerImpl_FreeRTOS::_StopEventLoopTask(void) +{ + VerifyOrDieWithMsg(false, DeviceLayer, "StopEventLoopTask is not implemented"); return CHIP_ERROR_NOT_IMPLEMENTED; } diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.h b/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.h index 0d4b83b201d6f2..24c5a65a4d864c 100644 --- a/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.h +++ b/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.h @@ -71,6 +71,7 @@ class GenericPlatformManagerImpl_FreeRTOS : public GenericPlatformManagerImpl #include #include @@ -64,6 +65,7 @@ template CHIP_ERROR GenericPlatformManagerImpl_POSIX::_InitChipStack() { CHIP_ERROR err = CHIP_NO_ERROR; + int ret = 0; mChipStackLock = PTHREAD_MUTEX_INITIALIZER; @@ -73,7 +75,20 @@ CHIP_ERROR GenericPlatformManagerImpl_POSIX::_InitChipStack() mShouldRunEventLoop.store(true, std::memory_order_relaxed); + ret = pthread_cond_init(&mEventQueueStoppedCond, nullptr); + SuccessOrExit(ret); + + ret = pthread_mutex_init(&mStateLock, nullptr); + SuccessOrExit(ret); + + mHasValidChipTask = false; + exit: + if (ret != 0) + { + err = System::MapErrorPOSIX(ret); + } + return err; } @@ -220,6 +235,23 @@ void GenericPlatformManagerImpl_POSIX::SysProcess() template void GenericPlatformManagerImpl_POSIX::_RunEventLoop() { + pthread_mutex_lock(&mStateLock); + + // + // If we haven't set mHasValidChipTask by now, it means that the application did not call StartEventLoopTask + // and consequently, are running the event loop from their own, externally managed task. + // Let's track his appropriately since we need this info later when stopping the event queues. + // + if (!mHasValidChipTask) + { + mHasValidChipTask = true; + mChipTask = pthread_self(); + mTaskType = kExternallyManagedTask; + } + + mEventQueueHasStopped = false; + pthread_mutex_unlock(&mStateLock); + Impl()->LockChipStack(); do @@ -229,6 +261,18 @@ void GenericPlatformManagerImpl_POSIX::_RunEventLoop() } while (mShouldRunEventLoop.load(std::memory_order_relaxed)); Impl()->UnlockChipStack(); + + pthread_mutex_lock(&mStateLock); + mEventQueueHasStopped = true; + pthread_mutex_unlock(&mStateLock); + + // + // Wake up anyone blocked waiting for the event queue to stop in + // StopEventLoopTask(). + // + pthread_cond_signal(&mEventQueueStoppedCond); + + return; } template @@ -250,29 +294,96 @@ CHIP_ERROR GenericPlatformManagerImpl_POSIX::_StartEventLoopTask() SuccessOrExit(err); err = pthread_attr_setschedpolicy(&mChipTaskAttr, SCHED_RR); SuccessOrExit(err); + + // + // We need to grab the lock here since we have to protect setting + // mHasValidChipTask, which will be read right away upon creating the + // thread below. + // + pthread_mutex_lock(&mStateLock); + err = pthread_create(&mChipTask, &mChipTaskAttr, EventLoopTaskMain, this); - SuccessOrExit(err); + if (err == 0) + { + mHasValidChipTask = true; + mTaskType = kInternallyManagedTask; + } + + pthread_mutex_unlock(&mStateLock); + exit: return System::MapErrorPOSIX(err); } template -CHIP_ERROR GenericPlatformManagerImpl_POSIX::_Shutdown() +CHIP_ERROR GenericPlatformManagerImpl_POSIX::_StopEventLoopTask() { int err = 0; + + // + // Signal to the runloop to stop. + // mShouldRunEventLoop.store(false, std::memory_order_relaxed); - if (mChipTask) + + pthread_mutex_lock(&mStateLock); + + // + // If we're calling this from a different thread than the one running chip, then + // we need to wait till the event queue has completely stopped before proceeding. + // + if (mHasValidChipTask && (pthread_equal(pthread_self(), mChipTask) == 0)) { + pthread_mutex_unlock(&mStateLock); + + // + // We need to grab the lock to protect critical sections accessed by the WakeSelect() call within + // SystemLayer. + // + Impl()->LockChipStack(); SystemLayer.WakeSelect(); - SuccessOrExit(err = pthread_join(mChipTask, nullptr)); + Impl()->UnlockChipStack(); + + pthread_mutex_lock(&mStateLock); + + while (!mEventQueueHasStopped) + { + err = pthread_cond_wait(&mEventQueueStoppedCond, &mStateLock); + SuccessOrExit(err); + } + + pthread_mutex_unlock(&mStateLock); + + // + // Wait further for the thread to terminate if we had previously created it. + // + if (mTaskType == kInternallyManagedTask) + { + err = pthread_join(mChipTask, nullptr); + SuccessOrExit(err); + } + } + else + { + pthread_mutex_unlock(&mStateLock); } - // Call up to the base class _Shutdown() to perform the bulk of the shutdown. - err = GenericPlatformManagerImpl::_Shutdown(); exit: + pthread_mutex_destroy(&mStateLock); + pthread_cond_destroy(&mEventQueueStoppedCond); + mHasValidChipTask = false; return System::MapErrorPOSIX(err); } +template +CHIP_ERROR GenericPlatformManagerImpl_POSIX::_Shutdown() +{ + // + // Call up to the base class _Shutdown() to perform the actual stack de-initialization + // and clean-up + // + return System::MapErrorPOSIX(GenericPlatformManagerImpl::_Shutdown()); +} + // Fully instantiate the generic implementation class in whatever compilation unit includes this file. template class GenericPlatformManagerImpl_POSIX; diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.h b/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.h index 4f5290e9063e2b..1896c52baa8ef1 100644 --- a/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.h +++ b/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.h @@ -63,7 +63,24 @@ class GenericPlatformManagerImpl_POSIX : public GenericPlatformManagerImpl mChipEventQueue; + enum TaskType + { + kExternallyManagedTask = 0, + kInternallyManagedTask = 1 + }; + pthread_t mChipTask; + bool mHasValidChipTask = false; + TaskType mTaskType; + pthread_cond_t mEventQueueStoppedCond; + pthread_mutex_t mStateLock; + + // + // TODO: This variable is very similar to mMainLoopIsStarted, track the + // cleanup and consolidation in this issue: + // + bool mEventQueueHasStopped = false; + pthread_attr_t mChipTaskAttr; struct sched_param mChipTaskSchedParam; @@ -83,6 +100,7 @@ class GenericPlatformManagerImpl_POSIX : public GenericPlatformManagerImpl::_StartChipTimer(uint32_ return CHIP_NO_ERROR; } +template +CHIP_ERROR GenericPlatformManagerImpl_Zephyr::_StopEventLoopTask(void) +{ + VerifyOrDieWithMsg(false, DeviceLayer, "StopEventLoopTask is not implemented"); + return CHIP_ERROR_NOT_IMPLEMENTED; +} + template CHIP_ERROR GenericPlatformManagerImpl_Zephyr::_Shutdown(void) { + VerifyOrDieWithMsg(false, DeviceLayer, "Shutdown is not implemented"); return CHIP_ERROR_NOT_IMPLEMENTED; } diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_Zephyr.h b/src/include/platform/internal/GenericPlatformManagerImpl_Zephyr.h index 6e26e5d394a942..1ac68adae4955e 100644 --- a/src/include/platform/internal/GenericPlatformManagerImpl_Zephyr.h +++ b/src/include/platform/internal/GenericPlatformManagerImpl_Zephyr.h @@ -76,6 +76,7 @@ class GenericPlatformManagerImpl_Zephyr : public GenericPlatformManagerImpl Date: Fri, 11 Jun 2021 12:19:58 -0700 Subject: [PATCH 2/3] Restyler fixes --- examples/chip-tool/commands/clusters/ModelCommand.cpp | 2 +- examples/chip-tool/commands/discover/DiscoverCommand.cpp | 2 +- examples/chip-tool/commands/reporting/ReportingCommand.cpp | 2 +- examples/chip-tool/commands/tests/TestCommand.cpp | 2 +- src/controller/CHIPDeviceController.cpp | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/examples/chip-tool/commands/clusters/ModelCommand.cpp b/examples/chip-tool/commands/clusters/ModelCommand.cpp index 55582dcca25892..632304580563aa 100644 --- a/examples/chip-tool/commands/clusters/ModelCommand.cpp +++ b/examples/chip-tool/commands/clusters/ModelCommand.cpp @@ -47,7 +47,7 @@ CHIP_ERROR ModelCommand::Run(NodeId localId, NodeId remoteId) // set the variable to true, which will cause it to block indefinitely. // UpdateWaitForResponse(true); - + { chip::DeviceLayer::StackLock lock; diff --git a/examples/chip-tool/commands/discover/DiscoverCommand.cpp b/examples/chip-tool/commands/discover/DiscoverCommand.cpp index 506b460b16c856..e1b05acc14edb7 100644 --- a/examples/chip-tool/commands/discover/DiscoverCommand.cpp +++ b/examples/chip-tool/commands/discover/DiscoverCommand.cpp @@ -30,7 +30,7 @@ CHIP_ERROR DiscoverCommand::Run(NodeId localId, NodeId remoteId) // set the variable to true, which will cause it to block indefinitely. // UpdateWaitForResponse(true); - + { chip::DeviceLayer::StackLock lock; diff --git a/examples/chip-tool/commands/reporting/ReportingCommand.cpp b/examples/chip-tool/commands/reporting/ReportingCommand.cpp index 4612301e8d07b4..263d10730c2db1 100644 --- a/examples/chip-tool/commands/reporting/ReportingCommand.cpp +++ b/examples/chip-tool/commands/reporting/ReportingCommand.cpp @@ -39,7 +39,7 @@ CHIP_ERROR ReportingCommand::Run(NodeId localId, NodeId remoteId) // set the variable to true, which will cause it to block indefinitely. // UpdateWaitForResponse(true); - + { chip::DeviceLayer::StackLock lock; diff --git a/examples/chip-tool/commands/tests/TestCommand.cpp b/examples/chip-tool/commands/tests/TestCommand.cpp index e7f09ffc45ebcc..ca7854eeb732d3 100644 --- a/examples/chip-tool/commands/tests/TestCommand.cpp +++ b/examples/chip-tool/commands/tests/TestCommand.cpp @@ -30,7 +30,7 @@ CHIP_ERROR TestCommand::Run(NodeId localId, NodeId remoteId) // set the variable to true, which will cause it to block indefinitely. // UpdateWaitForResponse(true); - + { chip::DeviceLayer::StackLock lock; diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index c95f41fc148193..c00e945be1624d 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -297,7 +297,7 @@ CHIP_ERROR DeviceController::Shutdown() // stack lock. // // Actually stopping the event queue is a separable call that applications will have to sequence. - // Consumers are expected to call PlaformMgr().StopEventLoopTask() before calling + // Consumers are expected to call PlaformMgr().StopEventLoopTask() before calling // DeviceController::Shutdown() in the CONFIG_DEVICE_LAYER configuration // ReturnErrorOnFailure(DeviceLayer::PlatformMgr().Shutdown()); From e60afe432e3dee09ec1fd079ec31813b91841029 Mon Sep 17 00:00:00 2001 From: Jerry Johns Date: Fri, 11 Jun 2021 12:55:36 -0700 Subject: [PATCH 3/3] Forgot a file.. --- .../chip-tool/templates/partials/test_cluster.zapt | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/examples/chip-tool/templates/partials/test_cluster.zapt b/examples/chip-tool/templates/partials/test_cluster.zapt index 2a201787bb4525..5d461c438c043a 100644 --- a/examples/chip-tool/templates/partials/test_cluster.zapt +++ b/examples/chip-tool/templates/partials/test_cluster.zapt @@ -2,7 +2,7 @@ class {{asCamelCased filename false}}: public TestCommand { public: - {{asCamelCased filename false}}(): TestCommand("{{filename}}") {} + {{asCamelCased filename false}}(): TestCommand("{{filename}}"), mTestIndex(0) {} /////////// TestCommand Interface ///////// CHIP_ERROR NextTest() override @@ -15,7 +15,11 @@ class {{asCamelCased filename false}}: public TestCommand SetCommandExitStatus(true); } - switch (mTestIndex) + // Ensure we increment mTestIndex before we start running the relevant + // command. That way if we lose the timeslice after we send the message + // but before our function call returns, we won't end up with an + // incorrect mTestIndex value observed when we get the response. + switch (mTestIndex++) { {{#chip_tests_items}} case {{index}}: @@ -23,7 +27,6 @@ class {{asCamelCased filename false}}: public TestCommand break; {{/chip_tests_items}} } - mTestIndex++; if (CHIP_NO_ERROR != err) { @@ -36,8 +39,8 @@ class {{asCamelCased filename false}}: public TestCommand private: - uint16_t mTestIndex = 0; - uint16_t mTestCount = {{totalTests}}; + std::atomic_uint16_t mTestIndex; + const uint16_t mTestCount = {{totalTests}}; // // Tests methods