From 2282872e5f46181c043775a3dcc6a14fc483f65b Mon Sep 17 00:00:00 2001 From: tehampson Date: Mon, 2 May 2022 16:24:58 -0400 Subject: [PATCH] Handle response errors during commissioning (#17778) * Handle ArmFailSafe response error propgating to AutoCommissioner * format fixing * Fixes based on pull request comments * Fix issue inside AutoCommissioner * Remove map, as the errors were redundant * Finish off plumbing error codes and networkstatus to AutoCommissioner * OnSetRegulatoryConfigResponse * OnNetworkConfigResponse * OnConnectNetworkResponse * OnCommissioningCompleteResponse * Remove TODO about checking if static_cast is correct I saw an example were static_cast was used on another `enum class Asdf : uint8_t`, so this does seems like the appraoch used elsewhere in this codebase * Last little bit of cleanup to turn this into real pull request * Apply suggestions from code review Co-authored-by: Boris Zbarsky * Fix formatting issues Co-authored-by: Boris Zbarsky --- src/controller/AutoCommissioner.cpp | 30 ++++++---- src/controller/CHIPDeviceController.cpp | 76 ++++++++++++++++++------- src/controller/CommissioningDelegate.h | 41 +++++++++---- 3 files changed, 104 insertions(+), 43 deletions(-) diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index 1b7857a638ed7a..620d28c3443149 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -363,22 +363,28 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio { completionStatus.failedStage = MakeOptional(report.stageCompleted); ChipLogError(Controller, "Failed to perform commissioning step %d", static_cast(report.stageCompleted)); - if (report.stageCompleted == CommissioningStage::kAttestationVerification) + if (report.Is()) { - if (report.Is()) + completionStatus.attestationResult = MakeOptional(report.Get().attestationResult); + if ((report.Get().attestationResult == + Credentials::AttestationVerificationResult::kDacProductIdMismatch) || + (report.Get().attestationResult == + Credentials::AttestationVerificationResult::kDacVendorIdMismatch)) { - completionStatus.attestationResult = MakeOptional(report.Get().attestationResult); - if ((report.Get().attestationResult == - Credentials::AttestationVerificationResult::kDacProductIdMismatch) || - (report.Get().attestationResult == - Credentials::AttestationVerificationResult::kDacVendorIdMismatch)) - { - ChipLogError(Controller, - "Failed device attestation. Device vendor and/or product ID do not match the IDs expected. " - "Verify DAC certificate chain and certification declaration to ensure spec rules followed."); - } + ChipLogError(Controller, + "Failed device attestation. Device vendor and/or product ID do not match the IDs expected. " + "Verify DAC certificate chain and certification declaration to ensure spec rules followed."); } } + else if (report.Is()) + { + completionStatus.commissioningError = MakeOptional(report.Get().commissioningError); + } + else if (report.Is()) + { + completionStatus.networkCommissioningStatus = + MakeOptional(report.Get().networkCommissioningStatus); + } } else { diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index a2b18d9c4bd48b..c62755a15f7bd4 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -736,7 +736,7 @@ DeviceCommissioner::ContinueCommissioningAfterDeviceAttestationFailure(DevicePro to_underlying(attestationResult)); CommissioningDelegate::CommissioningReport report; - report.Set(attestationResult); + report.Set(attestationResult); CommissioningStageComplete(CHIP_ERROR_INTERNAL, report); } else @@ -914,7 +914,7 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification(void * conte if (result != AttestationVerificationResult::kSuccess) { CommissioningDelegate::CommissioningReport report; - report.Set(result); + report.Set(result); if (result == AttestationVerificationResult::kNotImplemented) { ChipLogError(Controller, @@ -975,7 +975,7 @@ void DeviceCommissioner::OnArmFailSafeExtendedForFailedDeviceAttestation( { ChipLogProgress(Controller, "Device attestation failed and no delegate set, failing commissioning"); CommissioningDelegate::CommissioningReport report; - report.Set(commissioner->mAttestationResult); + report.Set(commissioner->mAttestationResult); commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL, report); } } @@ -986,7 +986,7 @@ void DeviceCommissioner::OnFailedToExtendedArmFailSafeFailedDeviceAttestation(vo DeviceCommissioner * commissioner = static_cast(context); CommissioningDelegate::CommissioningReport report; - report.Set(commissioner->mAttestationResult); + report.Set(commissioner->mAttestationResult); commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL, report); } @@ -1455,6 +1455,8 @@ void DeviceCommissioner::SendCommissioningCompleteCallbacks(NodeId nodeId, const } else { + // TODO: We should propogate detailed error information (commissioningError, networkCommissioningStatus) from + // completionStatus. mPairingDelegate->OnCommissioningFailure(peerId, completionStatus.err, completionStatus.failedStage.ValueOr(kError), completionStatus.attestationResult); } @@ -1710,46 +1712,82 @@ void DeviceCommissioner::OnDone() void DeviceCommissioner::OnArmFailSafe(void * context, const GeneralCommissioning::Commands::ArmFailSafeResponse::DecodableType & data) { - // TODO: Use errorCode - ChipLogProgress(Controller, "Received ArmFailSafe response"); + CommissioningDelegate::CommissioningReport report; + CHIP_ERROR err = CHIP_NO_ERROR; + + ChipLogProgress(Controller, "Received ArmFailSafe response errorCode=%u", to_underlying(data.errorCode)); + if (data.errorCode != GeneralCommissioning::CommissioningError::kOk) + { + err = CHIP_ERROR_INTERNAL; + report.Set(data.errorCode); + } + DeviceCommissioner * commissioner = static_cast(context); - commissioner->CommissioningStageComplete(CHIP_NO_ERROR); + commissioner->CommissioningStageComplete(err, report); } void DeviceCommissioner::OnSetRegulatoryConfigResponse( void * context, const GeneralCommissioning::Commands::SetRegulatoryConfigResponse::DecodableType & data) { - // TODO: Use errorCode - ChipLogProgress(Controller, "Received SetRegulatoryConfig response"); + CommissioningDelegate::CommissioningReport report; + CHIP_ERROR err = CHIP_NO_ERROR; + + ChipLogProgress(Controller, "Received SetRegulatoryConfig response errorCode=%u", to_underlying(data.errorCode)); + if (data.errorCode != GeneralCommissioning::CommissioningError::kOk) + { + err = CHIP_ERROR_INTERNAL; + report.Set(data.errorCode); + } DeviceCommissioner * commissioner = static_cast(context); - commissioner->CommissioningStageComplete(CHIP_NO_ERROR); + commissioner->CommissioningStageComplete(err, report); } void DeviceCommissioner::OnNetworkConfigResponse(void * context, const NetworkCommissioning::Commands::NetworkConfigResponse::DecodableType & data) { - // TODO: Use networkingStatus - ChipLogProgress(Controller, "Received NetworkConfig response"); + CommissioningDelegate::CommissioningReport report; + CHIP_ERROR err = CHIP_NO_ERROR; + + ChipLogProgress(Controller, "Received NetworkConfig response, networkingStatus=%u", to_underlying(data.networkingStatus)); + if (data.networkingStatus != NetworkCommissioning::NetworkCommissioningStatus::kSuccess) + { + err = CHIP_ERROR_INTERNAL; + report.Set(data.networkingStatus); + } DeviceCommissioner * commissioner = static_cast(context); - commissioner->CommissioningStageComplete(CHIP_NO_ERROR); + commissioner->CommissioningStageComplete(err, report); } void DeviceCommissioner::OnConnectNetworkResponse( void * context, const NetworkCommissioning::Commands::ConnectNetworkResponse::DecodableType & data) { - // TODO: Use networkingStatus - ChipLogProgress(Controller, "Received ConnectNetwork response"); + CommissioningDelegate::CommissioningReport report; + CHIP_ERROR err = CHIP_NO_ERROR; + + ChipLogProgress(Controller, "Received ConnectNetwork response, networkingStatus=%u", to_underlying(data.networkingStatus)); + if (data.networkingStatus != NetworkCommissioning::NetworkCommissioningStatus::kSuccess) + { + err = CHIP_ERROR_INTERNAL; + report.Set(data.networkingStatus); + } DeviceCommissioner * commissioner = static_cast(context); - commissioner->CommissioningStageComplete(CHIP_NO_ERROR); + commissioner->CommissioningStageComplete(err, report); } void DeviceCommissioner::OnCommissioningCompleteResponse( void * context, const GeneralCommissioning::Commands::CommissioningCompleteResponse::DecodableType & data) { - // TODO: Use errorCode - ChipLogProgress(Controller, "Received CommissioningComplete response"); + CommissioningDelegate::CommissioningReport report; + CHIP_ERROR err = CHIP_NO_ERROR; + + ChipLogProgress(Controller, "Received CommissioningComplete response, errorCode=%u", to_underlying(data.errorCode)); + if (data.errorCode != GeneralCommissioning::CommissioningError::kOk) + { + err = CHIP_ERROR_INTERNAL; + report.Set(data.errorCode); + } DeviceCommissioner * commissioner = static_cast(context); - commissioner->CommissioningStageComplete(CHIP_NO_ERROR); + commissioner->CommissioningStageComplete(err, report); } void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, CommissioningStage step, CommissioningParameters & params, diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index f176576c065dce..666b0c7e8a8feb 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -74,6 +74,8 @@ struct CompletionStatus CHIP_ERROR err; Optional failedStage; Optional attestationResult; + Optional commissioningError; + Optional networkCommissioningStatus; }; constexpr uint16_t kDefaultFailsafeTimeout = 60; @@ -447,38 +449,53 @@ struct ReadCommissioningInfo GeneralCommissioningInfo general; }; -struct AdditionalErrorInfo +struct AttestationErrorInfo { - AdditionalErrorInfo(Credentials::AttestationVerificationResult result) : attestationResult(result) {} + AttestationErrorInfo(Credentials::AttestationVerificationResult result) : attestationResult(result) {} Credentials::AttestationVerificationResult attestationResult; }; +struct CommissioningErrorInfo +{ + CommissioningErrorInfo(app::Clusters::GeneralCommissioning::CommissioningError result) : commissioningError(result) {} + app::Clusters::GeneralCommissioning::CommissioningError commissioningError; +}; + +struct NetworkCommissioningStatusInfo +{ + NetworkCommissioningStatusInfo(app::Clusters::NetworkCommissioning::NetworkCommissioningStatus result) : + networkCommissioningStatus(result) + {} + app::Clusters::NetworkCommissioning::NetworkCommissioningStatus networkCommissioningStatus; +}; + class CommissioningDelegate { public: virtual ~CommissioningDelegate(){}; /* CommissioningReport is returned after each commissioning step is completed. The reports for each step are: * kReadCommissioningInfo - ReadCommissioningInfo - * kArmFailsafe: none - * kConfigRegulatory: none + * kArmFailsafe: CommissioningErrorInfo if there is an error + * kConfigRegulatory: CommissioningErrorInfo if there is an error * kSendPAICertificateRequest: RequestedCertificate * kSendDACCertificateRequest: RequestedCertificate * kSendAttestationRequest: AttestationResponse - * kAttestationVerification: AdditionalErrorInfo if there is an error + * kAttestationVerification: AttestationErrorInfo if there is an error * kSendOpCertSigningRequest: CSRResponse * kGenerateNOCChain: NocChain * kSendTrustedRootCert: None * kSendNOC: none - * kWiFiNetworkSetup: none - * kThreadNetworkSetup: none - * kWiFiNetworkEnable: none - * kThreadNetworkEnable: none + * kWiFiNetworkSetup: NetworkCommissioningStatusInfo if there is an error + * kThreadNetworkSetup: NetworkCommissioningStatusInfo if there is an error + * kWiFiNetworkEnable: NetworkCommissioningStatusInfo if there is an error + * kThreadNetworkEnable: NetworkCommissioningStatusInfo if there is an error * kFindOperational: OperationalNodeFoundData - * kSendComplete: none + * kSendComplete: CommissioningErrorInfo if there is an error * kCleanup: none */ - struct CommissioningReport : Variant + struct CommissioningReport + : Variant { CommissioningReport() : stageCompleted(CommissioningStage::kError) {} CommissioningStage stageCompleted;