From 7b5634ca42e6310249b4dbdeb74934b28f80b73d Mon Sep 17 00:00:00 2001 From: Jerry Johns Date: Wed, 9 Mar 2022 16:52:52 -0800 Subject: [PATCH] Review feedback + some more prints. --- src/app/CASESessionManager.cpp | 2 +- src/app/OperationalDeviceProxy.cpp | 6 +- src/controller/AutoCommissioner.cpp | 94 ----------------- src/controller/AutoCommissioner.h | 1 - src/controller/BUILD.gn | 1 + src/controller/CHIPDeviceController.cpp | 13 +-- src/controller/CommissioningDelegate.cpp | 115 +++++++++++++++++++++ src/controller/CommissioningDelegate.h | 2 + src/lib/dnssd/Discovery_ImplPlatform.cpp | 3 + src/lib/dnssd/Resolver.h | 2 +- src/lib/dnssd/Resolver_ImplMinimalMdns.cpp | 2 + 11 files changed, 131 insertions(+), 110 deletions(-) create mode 100644 src/controller/CommissioningDelegate.cpp diff --git a/src/app/CASESessionManager.cpp b/src/app/CASESessionManager.cpp index 71a453c01b7883..30a1fd58ba86f1 100644 --- a/src/app/CASESessionManager.cpp +++ b/src/app/CASESessionManager.cpp @@ -45,7 +45,7 @@ CHIP_ERROR CASESessionManager::FindOrEstablishSession(PeerId peerId, Callback::C #endif ChipLogDetail(CASESessionManager, - "FindOrEstablishSession: PeerId = CF" ChipLogFormatX64 ":N" ChipLogFormatX64 ", NodeIdWasResolved = %d", + "FindOrEstablishSession: PeerId = " ChipLogFormatX64 ":" ChipLogFormatX64 ", NodeIdWasResolved = %d", ChipLogValueX64(peerId.GetCompressedFabricId()), ChipLogValueX64(peerId.GetNodeId()), nodeIDWasResolved); OperationalDeviceProxy * session = FindExistingSession(peerId); diff --git a/src/app/OperationalDeviceProxy.cpp b/src/app/OperationalDeviceProxy.cpp index 584eb18fb35437..555463bb3fdea5 100644 --- a/src/app/OperationalDeviceProxy.cpp +++ b/src/app/OperationalDeviceProxy.cpp @@ -47,9 +47,9 @@ void OperationalDeviceProxy::MoveToState(State aTargetState) if (mState != aTargetState) { ChipLogDetail(Controller, - "OperationalDeviceProxy[CF" ChipLogFormatX64 "-" ChipLogFormatX64 "]: Moving from state %d --> %d", - ChipLogValueX64(mPeerId.GetCompressedFabricId()), ChipLogValueX64(mPeerId.GetNodeId()), (uint32_t) mState, - (uint32_t) aTargetState); + "OperationalDeviceProxy[" ChipLogFormatX64 ":" ChipLogFormatX64 "]: State change %" PRIu32 " --> %" PRIu32, + ChipLogValueX64(mPeerId.GetCompressedFabricId()), ChipLogValueX64(mPeerId.GetNodeId()), to_underlying(mState), + to_underlying(aTargetState)); mState = aTargetState; } } diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index 6fa71f51765151..938e1b4e79187d 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -313,100 +313,6 @@ CHIP_ERROR AutoCommissioner::NOCChainGenerated(ByteSpan noc, ByteSpan icac, Byte return CHIP_NO_ERROR; } -const char * AutoCommissioner::StageToString(CommissioningStage stage) -{ - switch (stage) - { - case kError: - return "Error"; - break; - - case kSecurePairing: - return "SecurePairing"; - break; - - case kArmFailsafe: - return "ArmFailSafe"; - break; - - case kGetNetworkTechnology: - return "GetNetworkTechnology"; - break; - - case kConfigRegulatory: - return "ConfigRegulatory"; - break; - - case kSendPAICertificateRequest: - return "SendPAICertificateRequest"; - break; - - case kSendDACCertificateRequest: - return "SendDACCertificateRequest"; - break; - - case kSendAttestationRequest: - return "SendAttestationRequest"; - break; - - case kAttestationVerification: - return "AttestationVerification"; - break; - - case kSendOpCertSigningRequest: - return "SendOpCertSigningRequest"; - break; - - case kGenerateNOCChain: - return "GenerateNOCChain"; - break; - - case kSendTrustedRootCert: - return "SendTrustedRootCert"; - break; - - case kSendNOC: - return "SendNOC"; - break; - - case kWiFiNetworkSetup: - return "WiFiNetworkSetup"; - break; - - case kThreadNetworkSetup: - return "ThreadNetworkSetup"; - break; - - case kWiFiNetworkEnable: - return "WiFiNetworkEnable"; - break; - - case kThreadNetworkEnable: - return "ThreadNetworkEnable"; - break; - - case kFindOperational: - return "FindOperational"; - break; - - case kSendComplete: - return "SendComplete"; - break; - - case kCleanup: - return "Cleanup"; - break; - - case kConfigACL: - return "ConfigACL"; - break; - - default: - return "???"; - break; - } -} - CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, CommissioningDelegate::CommissioningReport report) { if (err != CHIP_NO_ERROR) diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h index 6060e9e1aa9afd..0e50957d577626 100644 --- a/src/controller/AutoCommissioner.h +++ b/src/controller/AutoCommissioner.h @@ -36,7 +36,6 @@ class AutoCommissioner : public CommissioningDelegate virtual CHIP_ERROR StartCommissioning(DeviceCommissioner * commissioner, CommissioneeDeviceProxy * proxy) override; virtual CHIP_ERROR CommissioningStepFinished(CHIP_ERROR err, CommissioningDelegate::CommissioningReport report) override; - static const char * StageToString(CommissioningStage stage); private: CommissioningStage GetNextCommissioningStage(CommissioningStage currentStage, CHIP_ERROR & lastErr); diff --git a/src/controller/BUILD.gn b/src/controller/BUILD.gn index 111b1573962082..02002d2290af05 100644 --- a/src/controller/BUILD.gn +++ b/src/controller/BUILD.gn @@ -44,6 +44,7 @@ static_library("controller") { "CHIPDeviceControllerFactory.h", "CommissioneeDeviceProxy.cpp", "CommissioneeDeviceProxy.h", + "CommissioningDelegate.cpp", "CommissionerDiscoveryController.cpp", "CommissionerDiscoveryController.h", "DeviceDiscoveryDelegate.h", diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 128f0a7588e7e3..d0152a17a08e89 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1446,8 +1446,8 @@ void DeviceCommissioner::CommissioningStageComplete(CHIP_ERROR err, Commissionin void DeviceCommissioner::OnOperationalNodeResolved(const chip::Dnssd::ResolvedNodeData & nodeData) { - ChipLogProgress(Controller, "F" ChipLogFormatX64 ": OperationalDiscoveryComplete for device ID N" ChipLogFormatX64, - ChipLogValueX64(mFabricId), ChipLogValueX64(nodeData.mPeerId.GetNodeId())); + ChipLogProgress(Controller, "[" ChipLogFormatX64 "] OperationalDiscoveryComplete for device " ChipLogFormatX64 ":" ChipLogFormatX64, + ChipLogValueX64(mFabricId), ChipLogValueX64(nodeData.mPeerId.GetCompressedFabricId()), ChipLogValueX64(nodeData.mPeerId.GetNodeId())); VerifyOrReturn(mState == State::Initialized); // TODO: minimal mdns is buggy and violates the API contract for the @@ -1752,7 +1752,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio Optional timeout) { ChipLogProgress(Controller, "Performing next commissioning step '%s' with completion status = '%s'", - AutoCommissioner::StageToString(step), params.GetCompletionStatus().AsString()); + StageToString(step), params.GetCompletionStatus().AsString()); // For now, we ignore errors coming in from the device since not all commissioning clusters are implemented on the device // side. @@ -1998,7 +1998,6 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); return; } - ChipLogProgress(Controller, "Sending operational certificate chain to the device"); SendOperationalCertificate(proxy, params.GetNoc().Value(), params.GetIcac().Value(), params.GetIpk().Value(), params.GetAdminSubject().Value()); break; @@ -2013,7 +2012,6 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio return; } - ChipLogProgress(Controller, "Adding wifi network"); NetworkCommissioning::Commands::AddOrUpdateWiFiNetwork::Type request; request.ssid = params.GetWiFiCredentials().Value().ssid; request.credentials = params.GetWiFiCredentials().Value().credentials; @@ -2028,7 +2026,6 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); return; } - ChipLogProgress(Controller, "Adding thread network"); NetworkCommissioning::Commands::AddOrUpdateThreadNetwork::Type request; request.operationalDataset = params.GetThreadOperationalDataset().Value(); request.breadcrumb = breadcrumb; @@ -2042,7 +2039,6 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); return; } - ChipLogProgress(Controller, "Enabling wifi network"); NetworkCommissioning::Commands::ConnectNetwork::Type request; request.networkID = params.GetWiFiCredentials().Value().ssid; request.breadcrumb = breadcrumb; @@ -2060,7 +2056,6 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); return; } - ChipLogProgress(Controller, "Enabling thread network"); NetworkCommissioning::Commands::ConnectNetwork::Type request; request.networkID = extendedPanId; request.breadcrumb = breadcrumb; @@ -2078,14 +2073,12 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio } break; case CommissioningStage::kSendComplete: { - ChipLogProgress(Controller, "Calling commissioning complete"); GeneralCommissioning::Commands::CommissioningComplete::Type request; SendCommand(proxy, request, OnCommissioningCompleteResponse, OnBasicFailure, endpoint, timeout); } break; case CommissioningStage::kCleanup: - ChipLogProgress(Controller, "Rendezvous cleanup"); if (mPairingDelegate != nullptr) { mPairingDelegate->OnCommissioningComplete(proxy->GetDeviceId(), params.GetCompletionStatus()); diff --git a/src/controller/CommissioningDelegate.cpp b/src/controller/CommissioningDelegate.cpp new file mode 100644 index 00000000000000..0eb9b432e54396 --- /dev/null +++ b/src/controller/CommissioningDelegate.cpp @@ -0,0 +1,115 @@ +/* + * + * Copyright (c) 2021 Project CHIP Authors + * 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. + */ + +#include + +namespace chip { +namespace Controller { + +const char *StageToString(CommissioningStage stage) +{ + switch (stage) + { + case kError: + return "Error"; + break; + + case kSecurePairing: + return "SecurePairing"; + break; + + case kArmFailsafe: + return "ArmFailSafe"; + break; + + case kConfigRegulatory: + return "ConfigRegulatory"; + break; + + case kSendPAICertificateRequest: + return "SendPAICertificateRequest"; + break; + + case kSendDACCertificateRequest: + return "SendDACCertificateRequest"; + break; + + case kSendAttestationRequest: + return "SendAttestationRequest"; + break; + + case kAttestationVerification: + return "AttestationVerification"; + break; + + case kSendOpCertSigningRequest: + return "SendOpCertSigningRequest"; + break; + + case kGenerateNOCChain: + return "GenerateNOCChain"; + break; + + case kSendTrustedRootCert: + return "SendTrustedRootCert"; + break; + + case kSendNOC: + return "SendNOC"; + break; + + case kWiFiNetworkSetup: + return "WiFiNetworkSetup"; + break; + + case kThreadNetworkSetup: + return "ThreadNetworkSetup"; + break; + + case kWiFiNetworkEnable: + return "WiFiNetworkEnable"; + break; + + case kThreadNetworkEnable: + return "ThreadNetworkEnable"; + break; + + case kFindOperational: + return "FindOperational"; + break; + + case kSendComplete: + return "SendComplete"; + break; + + case kCleanup: + return "Cleanup"; + break; + + case kConfigACL: + return "ConfigACL"; + break; + + default: + return "???"; + break; + } +} + +} // namespace Controller +} // namespace chip diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index b75168a9d47e3e..b7599ea9761021 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -55,6 +55,8 @@ enum CommissioningStage : uint8_t kConfigACL, }; +const char *StageToString(CommissioningStage stage); + struct WiFiCredentials { ByteSpan ssid; diff --git a/src/lib/dnssd/Discovery_ImplPlatform.cpp b/src/lib/dnssd/Discovery_ImplPlatform.cpp index efd3d81c62f117..2f09541e80ef6a 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.cpp +++ b/src/lib/dnssd/Discovery_ImplPlatform.cpp @@ -643,6 +643,9 @@ Resolver & chip::Dnssd::Resolver::Instance() CHIP_ERROR ResolverProxy::ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type) { VerifyOrReturnError(mDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE); + + ChipLogProgress(Discovery, "Resolving " ChipLogFormatX64 ":" ChipLogFormatX64" ...", ChipLogValueX64(peerId.GetCompressedFabricId()), ChipLogValueX64(peerId.GetNodeId())); + mDelegate->Retain(); DnssdService service; diff --git a/src/lib/dnssd/Resolver.h b/src/lib/dnssd/Resolver.h index 84c1f0b5d1eec9..ff2d3251b8b54e 100644 --- a/src/lib/dnssd/Resolver.h +++ b/src/lib/dnssd/Resolver.h @@ -73,7 +73,7 @@ struct ResolvedNodeData // Would be nice to log the interface id, but sorting out how to do so // across our differnet InterfaceId implementations is a pain. - ChipLogProgress(Discovery, "Node ID resolved for 0x" ChipLogFormatX64, ChipLogValueX64(mPeerId.GetNodeId())); + ChipLogProgress(Discovery, "Node ID resolved for " ChipLogFormatX64 ":" ChipLogFormatX64, ChipLogValueX64(mPeerId.GetCompressedFabricId()), ChipLogValueX64(mPeerId.GetNodeId())); for (unsigned i = 0; i < mNumIPs; ++i) { mAddress[i].ToString(addrBuffer); diff --git a/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp b/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp index 8336122f5be7a4..03d7031bb58f84 100644 --- a/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp +++ b/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp @@ -691,6 +691,8 @@ Resolver & chip::Dnssd::Resolver::Instance() CHIP_ERROR ResolverProxy::ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type) { VerifyOrReturnError(mDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE); + + ChipLogProgress(Discovery, "Resolving " ChipLogFormatX64 ":" ChipLogFormatX64" ...", ChipLogValueX64(peerId.GetCompressedFabricId()), ChipLogValueX64(peerId.GetNodeId())); chip::Dnssd::Resolver::Instance().SetOperationalDelegate(mDelegate); return chip::Dnssd::Resolver::Instance().ResolveNodeId(peerId, type); }