From 52d0b423a10e9af12cecce174da179a3208d6dd4 Mon Sep 17 00:00:00 2001 From: C Freeman Date: Tue, 8 Feb 2022 11:12:14 -0500 Subject: [PATCH] Bulk read all the things (#14752) * Bulk read all the things. * Use the recommended value to arm failsafe. * Whoopsies - not all c++17 * Update src/controller/AutoCommissioner.h Co-authored-by: Boris Zbarsky * Update src/controller/CHIPDeviceController.cpp Co-authored-by: Boris Zbarsky * Update src/controller/CHIPDeviceController.cpp Co-authored-by: Boris Zbarsky * Don't read all the attributes from basic. * Fixup errors from code suggestions. Get calls need explicit templating. Co-authored-by: Boris Zbarsky --- src/controller/AutoCommissioner.cpp | 59 ++++----- src/controller/AutoCommissioner.h | 5 +- src/controller/CHIPDeviceController.cpp | 154 ++++++++++++------------ src/controller/CommissioningDelegate.h | 50 ++++---- 4 files changed, 126 insertions(+), 142 deletions(-) diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index e46f2772e26fca..3795a15cc20339 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -106,21 +106,8 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag switch (currentStage) { case CommissioningStage::kSecurePairing: - return CommissioningStage::kReadVendorId; - case CommissioningStage::kReadVendorId: - return CommissioningStage::kReadProductId; - case CommissioningStage::kReadProductId: - return CommissioningStage::kReadSoftwareVersion; - case CommissioningStage::kReadSoftwareVersion: - if (mNeedsNetworkSetup) - { - return CommissioningStage::kGetNetworkTechnology; - } - else - { - return CommissioningStage::kArmFailsafe; - } - case CommissioningStage::kGetNetworkTechnology: + return CommissioningStage::kReadCommissioningInfo; + case CommissioningStage::kReadCommissioningInfo: return CommissioningStage::kArmFailsafe; case CommissioningStage::kArmFailsafe: return CommissioningStage::kConfigRegulatory; @@ -146,17 +133,24 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag // operational network because the provisioning of certificates will trigger the device to start operational advertising. if (mNeedsNetworkSetup) { - if (mParams.GetWiFiCredentials().HasValue() && mNetworkEndpoints.wifi != kInvalidEndpointId) + if (mParams.GetWiFiCredentials().HasValue() && mDeviceCommissioningInfo.network.wifi != kInvalidEndpointId) { return CommissioningStage::kWiFiNetworkSetup; } - else if (mParams.GetThreadOperationalDataset().HasValue() && mNetworkEndpoints.thread != kInvalidEndpointId) + else if (mParams.GetThreadOperationalDataset().HasValue() && + mDeviceCommissioningInfo.network.thread != kInvalidEndpointId) { return CommissioningStage::kThreadNetworkSetup; } else { ChipLogError(Controller, "Required network information not provided in commissioning parameters"); + ChipLogError(Controller, "Parameters supplied: wifi (%s) thread (%s)", + mParams.GetWiFiCredentials().HasValue() ? "yes" : "no", + mParams.GetThreadOperationalDataset().HasValue() ? "yes" : "no"); + ChipLogError(Controller, "Device supports: wifi (%s) thread(%s)", + mDeviceCommissioningInfo.network.wifi == kInvalidEndpointId ? "no" : "yes", + mDeviceCommissioningInfo.network.thread == kInvalidEndpointId ? "no" : "yes"); lastErr = CHIP_ERROR_INVALID_ARGUMENT; return CommissioningStage::kCleanup; } @@ -171,7 +165,7 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag #endif } case CommissioningStage::kWiFiNetworkSetup: - if (mParams.GetThreadOperationalDataset().HasValue() && mNetworkEndpoints.thread != kInvalidEndpointId) + if (mParams.GetThreadOperationalDataset().HasValue() && mDeviceCommissioningInfo.network.thread != kInvalidEndpointId) { return CommissioningStage::kThreadNetworkSetup; } @@ -180,7 +174,7 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag return CommissioningStage::kWiFiNetworkEnable; } case CommissioningStage::kThreadNetworkSetup: - if (mParams.GetWiFiCredentials().HasValue() && mNetworkEndpoints.wifi != kInvalidEndpointId) + if (mParams.GetWiFiCredentials().HasValue() && mDeviceCommissioningInfo.network.wifi != kInvalidEndpointId) { return CommissioningStage::kWiFiNetworkEnable; } @@ -190,7 +184,7 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag } case CommissioningStage::kWiFiNetworkEnable: - if (mParams.GetThreadOperationalDataset().HasValue() && mNetworkEndpoints.thread != kInvalidEndpointId) + if (mParams.GetThreadOperationalDataset().HasValue() && mDeviceCommissioningInfo.network.thread != kInvalidEndpointId) { return CommissioningStage::kThreadNetworkEnable; } @@ -227,12 +221,10 @@ EndpointId AutoCommissioner::GetEndpoint(const CommissioningStage & stage) { case CommissioningStage::kWiFiNetworkSetup: case CommissioningStage::kWiFiNetworkEnable: - return mNetworkEndpoints.wifi; + return mDeviceCommissioningInfo.network.wifi; case CommissioningStage::kThreadNetworkSetup: case CommissioningStage::kThreadNetworkEnable: - return mNetworkEndpoints.thread; - case CommissioningStage::kGetNetworkTechnology: - return kInvalidEndpointId; + return mDeviceCommissioningInfo.network.thread; default: return kRootEndpointId; } @@ -315,17 +307,12 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio { switch (report.stageCompleted) { - case CommissioningStage::kReadVendorId: - mVendorId = report.Get().vendorId; - break; - case CommissioningStage::kReadProductId: - mProductId = report.Get().productId; - break; - case CommissioningStage::kReadSoftwareVersion: - mSoftwareVersion = report.Get().softwareVersion; - break; - case CommissioningStage::kGetNetworkTechnology: - mNetworkEndpoints = report.Get(); + case CommissioningStage::kReadCommissioningInfo: + mDeviceCommissioningInfo = report.Get(); + if (!mParams.GetFailsafeTimerSeconds().HasValue() && mDeviceCommissioningInfo.general.recommendedFailsafe > 0) + { + mParams.SetFailsafeTimerSeconds(mDeviceCommissioningInfo.general.recommendedFailsafe); + } break; case CommissioningStage::kSendPAICertificateRequest: SetPAI(report.Get().certificate); @@ -367,7 +354,7 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio mCommissioneeDeviceProxy = nullptr; mOperationalDeviceProxy = nullptr; mParams = CommissioningParameters(); - mNetworkEndpoints = NetworkClusters(); + mDeviceCommissioningInfo = ReadCommissioningInfo(); return CHIP_NO_ERROR; default: break; diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h index 90438703ed9a29..199f201d0e2701 100644 --- a/src/controller/AutoCommissioner.h +++ b/src/controller/AutoCommissioner.h @@ -57,16 +57,13 @@ class AutoCommissioner : public CommissioningDelegate OperationalDeviceProxy * mOperationalDeviceProxy = nullptr; OperationalCredentialsDelegate * mOperationalCredentialsDelegate = nullptr; CommissioningParameters mParams = CommissioningParameters(); - VendorId mVendorId; - uint16_t mProductId; - uint32_t mSoftwareVersion; // Memory space for the commisisoning parameters that come in as ByteSpans - the caller is not guaranteed to retain this memory uint8_t mSsid[CommissioningParameters::kMaxSsidLen]; uint8_t mCredentials[CommissioningParameters::kMaxCredentialsLen]; uint8_t mThreadOperationalDataset[CommissioningParameters::kMaxThreadDatasetLen]; bool mNeedsNetworkSetup = false; - NetworkClusters mNetworkEndpoints; + ReadCommissioningInfo mDeviceCommissioningInfo; // TODO: Why were the nonces statically allocated, but the certs dynamically allocated? uint8_t * mDAC = nullptr; diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index f74d4cb8ef5d82..1e4fb605f75dc5 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1556,43 +1556,58 @@ void DeviceCommissioner::SetupCluster(ClusterBase & base, DeviceProxy * proxy, E base.SetCommandTimeout(timeout); } -void BasicVendorCallback(void * context, VendorId vendorId) +// AttributeCache::Callback impl +void DeviceCommissioner::OnDone() { - DeviceCommissioner * commissioner = static_cast(context); - CommissioningDelegate::CommissioningReport report; - report.Set(vendorId); - commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report); -} + CHIP_ERROR err; + CHIP_ERROR return_err = CHIP_NO_ERROR; + ReadCommissioningInfo info; -void BasicProductCallback(void * context, uint16_t productId) -{ - DeviceCommissioner * commissioner = static_cast(context); - CommissioningDelegate::CommissioningReport report; - report.Set(productId); - commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report); -} + // Using ForEachAttribute because this attribute can be queried on any endpoint. + err = mAttributeCache->ForEachAttribute( + app::Clusters::GeneralCommissioning::Id, [this, &info](const app::ConcreteAttributePath & path) { + if (path.mAttributeId != app::Clusters::GeneralCommissioning::Attributes::BasicCommissioningInfo::Id) + { + return CHIP_NO_ERROR; + } + app::Clusters::GeneralCommissioning::Attributes::BasicCommissioningInfo::TypeInfo::DecodableType basicInfo; + ReturnErrorOnFailure( + this->mAttributeCache->Get( + path, basicInfo)); + info.general.recommendedFailsafe = basicInfo.failSafeExpiryLengthSeconds; + return CHIP_NO_ERROR; + }); -void BasicSoftwareCallback(void * context, uint32_t softwareVersion) -{ - DeviceCommissioner * commissioner = static_cast(context); - CommissioningDelegate::CommissioningReport report; - report.Set(softwareVersion); - commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report); -} + // Try to parse as much as we can here before returning, even if this is an error. + return_err = err == CHIP_NO_ERROR ? return_err : err; -void AttributeReadFailure(void * context, CHIP_ERROR status) -{ - DeviceCommissioner * commissioner = static_cast(context); - commissioner->CommissioningStageComplete(status); -} + err = mAttributeCache->ForEachAttribute(app::Clusters::Basic::Id, [this, &info](const app::ConcreteAttributePath & path) { + if (path.mAttributeId != app::Clusters::Basic::Attributes::VendorID::Id && + path.mAttributeId != app::Clusters::Basic::Attributes::ProductID::Id && + path.mAttributeId != app::Clusters::Basic::Attributes::SoftwareVersion::Id) + { + // Continue on + return CHIP_NO_ERROR; + } -// AttributeCache::Callback impl -void DeviceCommissioner::OnDone() -{ - NetworkClusters clusters; + switch (path.mAttributeId) + { + case app::Clusters::Basic::Attributes::VendorID::Id: + return this->mAttributeCache->Get(path, info.basic.vendorId); + case app::Clusters::Basic::Attributes::ProductID::Id: + return this->mAttributeCache->Get(path, info.basic.productId); + case app::Clusters::Basic::Attributes::SoftwareVersion::Id: + return this->mAttributeCache->Get( + path, info.basic.softwareVersion); + default: + return CHIP_NO_ERROR; + } + }); + // Try to parse as much as we can here before returning, even if this is an error. + return_err = err == CHIP_NO_ERROR ? return_err : err; - CHIP_ERROR err = mAttributeCache->ForEachAttribute( - app::Clusters::NetworkCommissioning::Id, [this, &clusters](const app::ConcreteAttributePath & path) { + err = mAttributeCache->ForEachAttribute( + app::Clusters::NetworkCommissioning::Id, [this, &info](const app::ConcreteAttributePath & path) { if (path.mAttributeId != app::Clusters::NetworkCommissioning::Attributes::FeatureMap::Id) { return CHIP_NO_ERROR; @@ -1605,44 +1620,45 @@ void DeviceCommissioner::OnDone() { if (features.Has(app::Clusters::NetworkCommissioning::NetworkCommissioningFeature::kWiFiNetworkInterface)) { - clusters.wifi = path.mEndpointId; + info.network.wifi = path.mEndpointId; } else if (features.Has( app::Clusters::NetworkCommissioning::NetworkCommissioningFeature::kThreadNetworkInterface)) { - clusters.thread = path.mEndpointId; + info.network.thread = path.mEndpointId; } else if (features.Has( app::Clusters::NetworkCommissioning::NetworkCommissioningFeature::kEthernetNetworkInterface)) { - clusters.eth = path.mEndpointId; + info.network.eth = path.mEndpointId; } else { // TODO: Gross workaround for the empty feature map on all clusters. Remove. - if (clusters.thread == kInvalidEndpointId) + if (info.network.thread == kInvalidEndpointId) { - clusters.thread = path.mEndpointId; + info.network.thread = path.mEndpointId; } - if (clusters.wifi == kInvalidEndpointId) + if (info.network.wifi == kInvalidEndpointId) { - clusters.wifi = path.mEndpointId; + info.network.wifi = path.mEndpointId; } } } } return CHIP_NO_ERROR; }); + return_err = err == CHIP_NO_ERROR ? return_err : err; - if (err != CHIP_NO_ERROR) + if (return_err != CHIP_NO_ERROR) { - ChipLogError(Controller, "Error parsing Network commissioning features"); + ChipLogError(Controller, "Error parsing commissioning information"); } mAttributeCache = nullptr; mReadClient = nullptr; CommissioningDelegate::CommissioningReport report; - report.Set(clusters); - CommissioningStageComplete(err, report); + report.Set(info); + CommissioningStageComplete(return_err, report); } void DeviceCommissioner::OnArmFailSafe(void * context, const GeneralCommissioning::Commands::ArmFailSafeResponse::DecodableType & data) @@ -1707,48 +1723,36 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio switch (step) { - case CommissioningStage::kReadVendorId: { - ChipLogProgress(Controller, "Reading vendor ID"); - BasicCluster basic; - SetupCluster(basic, proxy, endpoint, timeout); - basic.ReadAttribute(this, BasicVendorCallback, - AttributeReadFailure); - } - break; - case CommissioningStage::kReadProductId: { - ChipLogProgress(Controller, "Reading product ID"); - BasicCluster basic; - SetupCluster(basic, proxy, endpoint, timeout); - basic.ReadAttribute(this, BasicProductCallback, - AttributeReadFailure); - } - break; - case CommissioningStage::kReadSoftwareVersion: { - ChipLogProgress(Controller, "Reading software version"); - BasicCluster basic; - SetupCluster(basic, proxy, endpoint, timeout); - basic.ReadAttribute(this, BasicSoftwareCallback, - AttributeReadFailure); - } - break; case CommissioningStage::kArmFailsafe: { - ChipLogProgress(Controller, "Arming failsafe"); - // TODO: should get the endpoint information from the descriptor cluster. GeneralCommissioning::Commands::ArmFailSafe::Type request; - request.expiryLengthSeconds = params.GetFailsafeTimerSeconds(); + request.expiryLengthSeconds = params.GetFailsafeTimerSeconds().ValueOr(kDefaultFailsafeTimeout); request.breadcrumb = breadcrumb; request.timeoutMs = kCommandTimeoutMs; + ChipLogProgress(Controller, "Arming failsafe (%u seconds)", request.expiryLengthSeconds); SendCommand(proxy, request, OnArmFailSafe, OnBasicFailure, endpoint, timeout); } break; - case CommissioningStage::kGetNetworkTechnology: { - ChipLogProgress(Controller, "Sending request for network cluster feature map"); + case CommissioningStage::kReadCommissioningInfo: { + ChipLogProgress(Controller, "Sending request for commissioning information"); app::InteractionModelEngine * engine = app::InteractionModelEngine::GetInstance(); app::ReadPrepareParams readParams(proxy->GetSecureSession().Value()); - app::AttributePathParams readPath(app::Clusters::NetworkCommissioning::Id, - app::Clusters::NetworkCommissioning::Attributes::FeatureMap::Id); - readParams.mpAttributePathParamsList = &readPath; - readParams.mAttributePathParamsListSize = 1; + + app::AttributePathParams readPaths[5]; + // Read all the feature maps for all the networking clusters on any endpoint to determine what is supported + readPaths[0] = app::AttributePathParams(app::Clusters::NetworkCommissioning::Id, + app::Clusters::NetworkCommissioning::Attributes::FeatureMap::Id); + // Get the basic commissioning info from the general commissioning cluster on this endpoint (recommended failsafe time) + readPaths[1] = app::AttributePathParams(endpoint, app::Clusters::GeneralCommissioning::Id, + app::Clusters::GeneralCommissioning::Attributes::BasicCommissioningInfo::Id); + // Read attributes from the basic info cluster (vendor id / product id / software version) + readPaths[2] = app::AttributePathParams(endpoint, app::Clusters::Basic::Id, app::Clusters::Basic::Attributes::VendorID::Id); + readPaths[3] = + app::AttributePathParams(endpoint, app::Clusters::Basic::Id, app::Clusters::Basic::Attributes::ProductID::Id); + readPaths[4] = + app::AttributePathParams(endpoint, app::Clusters::Basic::Id, app::Clusters::Basic::Attributes::SoftwareVersion::Id); + + readParams.mpAttributePathParamsList = readPaths; + readParams.mAttributePathParamsListSize = 5; if (timeout.HasValue()) { readParams.mTimeout = timeout.Value(); diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index 16474ee08460c1..db0fe09a21bfc0 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -30,10 +30,7 @@ enum CommissioningStage : uint8_t { kError, kSecurePairing, - kReadVendorId, - kReadProductId, - kReadSoftwareVersion, - kGetNetworkTechnology, + kReadCommissioningInfo, kArmFailsafe, // kConfigTime, // NOT YET IMPLEMENTED // kConfigTimeZone, // NOT YET IMPLEMENTED @@ -70,16 +67,16 @@ struct NOCChainGenerationParameters ByteSpan nocsrElements; ByteSpan signature; }; -struct NOCerts -{ -}; + +constexpr uint16_t kDefaultFailsafeTimeout = 60; class CommissioningParameters { public: static constexpr size_t kMaxThreadDatasetLen = 254; static constexpr size_t kMaxSsidLen = 32; static constexpr size_t kMaxCredentialsLen = 64; - uint16_t GetFailsafeTimerSeconds() const { return mFailsafeTimerSeconds; } + + const Optional GetFailsafeTimerSeconds() const { return mFailsafeTimerSeconds; } const Optional GetCSRNonce() const { return mCSRNonce; } const Optional GetAttestationNonce() const { return mAttestationNonce; } const Optional GetWiFiCredentials() const { return mWiFiCreds; } @@ -101,7 +98,7 @@ class CommissioningParameters CommissioningParameters & SetFailsafeTimerSeconds(uint16_t seconds) { - mFailsafeTimerSeconds = seconds; + mFailsafeTimerSeconds.SetValue(seconds); return *this; } @@ -188,7 +185,7 @@ class CommissioningParameters void SetCompletionStatus(CHIP_ERROR err) { completionStatus = err; } private: - uint16_t mFailsafeTimerSeconds = 60; + Optional mFailsafeTimerSeconds; Optional mCSRNonce; ///< CSR Nonce passed by the commissioner Optional mAttestationNonce; ///< Attestation Nonce passed by the commissioner Optional mWiFiCreds; @@ -239,37 +236,36 @@ struct OperationalNodeFoundData OperationalDeviceProxy * operationalProxy; }; -struct BasicVendor +struct NetworkClusters { - BasicVendor(VendorId id) : vendorId(id) {} - VendorId vendorId; + EndpointId wifi = kInvalidEndpointId; + EndpointId thread = kInvalidEndpointId; + EndpointId eth = kInvalidEndpointId; }; - -struct BasicProduct +struct BasicClusterInfo { - BasicProduct(uint16_t id) : productId(id) {} - uint16_t productId; + VendorId vendorId = VendorId::Common; + uint16_t productId = 0; + uint32_t softwareVersion = 0; }; - -struct BasicSoftware +struct GeneralCommissioningInfo { - BasicSoftware(uint32_t version) : softwareVersion(version) {} - uint32_t softwareVersion; + uint16_t recommendedFailsafe = 0; }; -struct NetworkClusters +struct ReadCommissioningInfo { - EndpointId wifi = kInvalidEndpointId; - EndpointId thread = kInvalidEndpointId; - EndpointId eth = kInvalidEndpointId; + NetworkClusters network; + BasicClusterInfo basic; + GeneralCommissioningInfo general; }; class CommissioningDelegate { public: virtual ~CommissioningDelegate(){}; - struct CommissioningReport : Variant + struct CommissioningReport + : Variant { CommissioningReport() : stageCompleted(CommissioningStage::kError) {} CommissioningStage stageCompleted;