From 102679654ff73ce835b4be9ff7d897688f51d74e Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 18 Jul 2022 19:55:23 -0400 Subject: [PATCH] Address various review comments for Darwin OTA delegate bridge. (#20888) * Address various review comments for Darwin OTA delegate bridge. Fixes https://github.com/project-chip/connectedhomeip/issues/20763 * Address review comments. --- .../Framework/CHIP/MTROTAProviderDelegate.h | 2 +- .../CHIP/MTROTAProviderDelegateBridge.h | 10 +- .../CHIP/MTROTAProviderDelegateBridge.mm | 177 +++++++++--------- 3 files changed, 94 insertions(+), 95 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTROTAProviderDelegate.h b/src/darwin/Framework/CHIP/MTROTAProviderDelegate.h index 163957072dd62f..81aef65dbf5005 100644 --- a/src/darwin/Framework/CHIP/MTROTAProviderDelegate.h +++ b/src/darwin/Framework/CHIP/MTROTAProviderDelegate.h @@ -26,7 +26,7 @@ NS_ASSUME_NONNULL_BEGIN * All delegate methods will be called on the supplied Delegate Queue. */ @protocol MTROTAProviderDelegate -@optional +@required /** * Notify the delegate when query image command is received * diff --git a/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.h b/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.h index 8f01a6d253cdf0..2bd56bf4c186c6 100644 --- a/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.h +++ b/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.h @@ -42,19 +42,19 @@ class MTROTAProviderDelegateBridge : public chip::app::Clusters::OTAProviderDele const chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::NotifyUpdateApplied::DecodableType & commandData) override; private: - void ConvertToQueryImageParams( + static CHIP_ERROR ConvertToQueryImageParams( const chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::QueryImage::DecodableType & commandData, MTROtaSoftwareUpdateProviderClusterQueryImageParams * commandParams); - void ConvertFromQueryImageResponseParms( + static void ConvertFromQueryImageResponseParms( const MTROtaSoftwareUpdateProviderClusterQueryImageResponseParams * responseParams, chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::QueryImageResponse::Type & response); - void ConvertToApplyUpdateRequestParams( + static void ConvertToApplyUpdateRequestParams( const chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::ApplyUpdateRequest::DecodableType & commandData, MTROtaSoftwareUpdateProviderClusterApplyUpdateRequestParams * commandParams); - void ConvertFromApplyUpdateRequestResponseParms( + static void ConvertFromApplyUpdateRequestResponseParms( const MTROtaSoftwareUpdateProviderClusterApplyUpdateResponseParams * responseParams, chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::ApplyUpdateResponse::Type & response); - void ConvertToNotifyUpdateAppliedParams( + static void ConvertToNotifyUpdateAppliedParams( const chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::NotifyUpdateApplied::DecodableType & commandData, MTROtaSoftwareUpdateProviderClusterNotifyUpdateAppliedParams * commandParams); diff --git a/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm b/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm index c4ca0caf0238bf..17c990b4c53697 100644 --- a/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm +++ b/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm @@ -16,9 +16,12 @@ */ #import "MTROTAProviderDelegateBridge.h" +#import "NSDataSpanConversion.h" #include +#include #include +#include static NSInteger const kOtaProviderEndpoint = 0; @@ -41,34 +44,36 @@ const chip::app::ConcreteCommandPath & commandPath, const chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::QueryImage::DecodableType & commandData) { - // Make sure to hold on to the command handler and command path to be used in the completion block - __block chip::app::CommandHandler::Handle handle(commandObj); - __block chip::app::ConcreteCommandPath cachedCommandPath( - commandPath.mEndpointId, commandPath.mClusterId, commandPath.mCommandId); - id strongDelegate = mDelegate; - if ([strongDelegate respondsToSelector:@selector(handleQueryImage:completionHandler:)]) { - if (strongDelegate && mQueue) { - auto * commandParams = [[MTROtaSoftwareUpdateProviderClusterQueryImageParams alloc] init]; - ConvertToQueryImageParams(commandData, commandParams); - - dispatch_async(mQueue, ^{ - [strongDelegate handleQueryImage:commandParams - completionHandler:^(MTROtaSoftwareUpdateProviderClusterQueryImageResponseParams * _Nullable data, - NSError * _Nullable error) { - dispatch_async(chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue(), ^{ - chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::QueryImageResponse::Type response; - ConvertFromQueryImageResponseParms(data, response); - - chip::app::CommandHandler * handler = handle.Get(); - if (handler) { - handler->AddResponse(cachedCommandPath, response); - handle.Release(); - } - }); - }]; - }); + if (strongDelegate && mQueue) { + auto * commandParams = [[MTROtaSoftwareUpdateProviderClusterQueryImageParams alloc] init]; + CHIP_ERROR err = ConvertToQueryImageParams(commandData, commandParams); + if (err != CHIP_NO_ERROR) { + commandObj->AddStatus(commandPath, chip::Protocols::InteractionModel::Status::InvalidCommand); + return; } + + // Make sure to hold on to the command handler and command path to be used in the completion block + __block chip::app::CommandHandler::Handle handle(commandObj); + __block chip::app::ConcreteCommandPath cachedCommandPath( + commandPath.mEndpointId, commandPath.mClusterId, commandPath.mCommandId); + + dispatch_async(mQueue, ^{ + [strongDelegate handleQueryImage:commandParams + completionHandler:^(MTROtaSoftwareUpdateProviderClusterQueryImageResponseParams * _Nullable data, + NSError * _Nullable error) { + dispatch_async(chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue(), ^{ + chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::QueryImageResponse::Type response; + ConvertFromQueryImageResponseParms(data, response); + + chip::app::CommandHandler * handler = handle.Get(); + if (handler) { + handler->AddResponse(cachedCommandPath, response); + handle.Release(); + } + }); + }]; + }); } } @@ -82,29 +87,27 @@ commandPath.mEndpointId, commandPath.mClusterId, commandPath.mCommandId); id strongDelegate = mDelegate; - if ([strongDelegate respondsToSelector:@selector(handleApplyUpdateRequest:completionHandler:)]) { - if (strongDelegate && mQueue) { - auto * commandParams = [[MTROtaSoftwareUpdateProviderClusterApplyUpdateRequestParams alloc] init]; - ConvertToApplyUpdateRequestParams(commandData, commandParams); - - dispatch_async(mQueue, ^{ - [strongDelegate - handleApplyUpdateRequest:commandParams - completionHandler:^(MTROtaSoftwareUpdateProviderClusterApplyUpdateResponseParams * _Nullable data, - NSError * _Nullable error) { - dispatch_async(chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue(), ^{ - chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::ApplyUpdateResponse::Type response; - ConvertFromApplyUpdateRequestResponseParms(data, response); - - chip::app::CommandHandler * handler = handle.Get(); - if (handler) { - handler->AddResponse(cachedCommandPath, response); - handle.Release(); - } - }); - }]; - }); - } + if (strongDelegate && mQueue) { + auto * commandParams = [[MTROtaSoftwareUpdateProviderClusterApplyUpdateRequestParams alloc] init]; + ConvertToApplyUpdateRequestParams(commandData, commandParams); + + dispatch_async(mQueue, ^{ + [strongDelegate + handleApplyUpdateRequest:commandParams + completionHandler:^(MTROtaSoftwareUpdateProviderClusterApplyUpdateResponseParams * _Nullable data, + NSError * _Nullable error) { + dispatch_async(chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue(), ^{ + chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::ApplyUpdateResponse::Type response; + ConvertFromApplyUpdateRequestResponseParms(data, response); + + chip::app::CommandHandler * handler = handle.Get(); + if (handler) { + handler->AddResponse(cachedCommandPath, response); + handle.Release(); + } + }); + }]; + }); } } @@ -118,45 +121,44 @@ commandPath.mEndpointId, commandPath.mClusterId, commandPath.mCommandId); id strongDelegate = mDelegate; - if ([strongDelegate respondsToSelector:@selector(handleNotifyUpdateApplied:completionHandler:)]) { - if (strongDelegate && mQueue) { - auto * commandParams = [[MTROtaSoftwareUpdateProviderClusterNotifyUpdateAppliedParams alloc] init]; - ConvertToNotifyUpdateAppliedParams(commandData, commandParams); - - dispatch_async(mQueue, ^{ - [strongDelegate - handleNotifyUpdateApplied:commandParams - completionHandler:^(NSError * _Nullable error) { - dispatch_async(chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue(), ^{ - chip::app::CommandHandler * handler = handle.Get(); - if (handler) { - handler->AddStatus(cachedCommandPath, chip::Protocols::InteractionModel::Status::Success); - handle.Release(); - } - }); - }]; - }); - } + if (strongDelegate && mQueue) { + auto * commandParams = [[MTROtaSoftwareUpdateProviderClusterNotifyUpdateAppliedParams alloc] init]; + ConvertToNotifyUpdateAppliedParams(commandData, commandParams); + + dispatch_async(mQueue, ^{ + [strongDelegate + handleNotifyUpdateApplied:commandParams + completionHandler:^(NSError * _Nullable error) { + dispatch_async(chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue(), ^{ + chip::app::CommandHandler * handler = handle.Get(); + if (handler) { + handler->AddStatus(cachedCommandPath, chip::Protocols::InteractionModel::Status::Success); + handle.Release(); + } + }); + }]; + }); } } -void MTROTAProviderDelegateBridge::ConvertToQueryImageParams( +CHIP_ERROR MTROTAProviderDelegateBridge::ConvertToQueryImageParams( const chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::QueryImage::DecodableType & commandData, MTROtaSoftwareUpdateProviderClusterQueryImageParams * commandParams) { - commandParams.vendorId = [NSNumber numberWithInt:commandData.vendorId]; - commandParams.productId = [NSNumber numberWithInt:commandData.productId]; - commandParams.softwareVersion = [NSNumber numberWithInt:commandData.softwareVersion]; + commandParams.vendorId = [NSNumber numberWithUnsignedShort:commandData.vendorId]; + commandParams.productId = [NSNumber numberWithUnsignedShort:commandData.productId]; + commandParams.softwareVersion = [NSNumber numberWithUnsignedLong:commandData.softwareVersion]; auto iterator = commandData.protocolsSupported.begin(); NSMutableArray * protocolsSupported = [[NSMutableArray alloc] init]; while (iterator.Next()) { chip::app::Clusters::OtaSoftwareUpdateProvider::OTADownloadProtocol protocol = iterator.GetValue(); - [protocolsSupported addObject:[NSNumber numberWithInt:static_cast(protocol)]]; + [protocolsSupported addObject:[NSNumber numberWithInt:chip::to_underlying(protocol)]]; } - commandParams.protocolsSupported = [protocolsSupported copy]; + ReturnErrorOnFailure(iterator.GetStatus()); + commandParams.protocolsSupported = protocolsSupported; if (commandData.hardwareVersion.HasValue()) { - commandParams.hardwareVersion = [NSNumber numberWithInt:commandData.hardwareVersion.Value()]; + commandParams.hardwareVersion = [NSNumber numberWithUnsignedShort:commandData.hardwareVersion.Value()]; } if (commandData.location.HasValue()) { @@ -170,9 +172,9 @@ } if (commandData.metadataForProvider.HasValue()) { - commandParams.metadataForProvider = [NSData dataWithBytes:commandData.metadataForProvider.Value().data() - length:commandData.metadataForProvider.Value().size()]; + commandParams.metadataForProvider = AsData(commandData.metadataForProvider.Value()); } + return CHIP_NO_ERROR; } void MTROTAProviderDelegateBridge::ConvertFromQueryImageResponseParms( @@ -182,7 +184,7 @@ response.status = static_cast([responseParams.status intValue]); if (responseParams.delayedActionTime) { - response.delayedActionTime.SetValue([responseParams.delayedActionTime intValue]); + response.delayedActionTime.SetValue([responseParams.delayedActionTime unsignedIntValue]); } if (responseParams.imageURI) { @@ -190,7 +192,7 @@ } if (responseParams.softwareVersion) { - response.softwareVersion.SetValue([responseParams.softwareVersion intValue]); + response.softwareVersion.SetValue([responseParams.softwareVersion unsignedIntValue]); } if (responseParams.softwareVersionString) { @@ -199,8 +201,7 @@ } if (responseParams.updateToken) { - UInt8 * updateTokenBytes = (UInt8 *) responseParams.updateToken.bytes; - response.updateToken.SetValue(chip::ByteSpan(updateTokenBytes, responseParams.updateToken.length)); + response.updateToken.SetValue(AsByteSpan(responseParams.updateToken)); } if (responseParams.userConsentNeeded) { @@ -208,9 +209,7 @@ } if (responseParams.metadataForRequestor) { - UInt8 * metadataForRequestorBytes = (UInt8 *) responseParams.metadataForRequestor.bytes; - response.metadataForRequestor.SetValue( - chip::ByteSpan(metadataForRequestorBytes, responseParams.metadataForRequestor.length)); + response.metadataForRequestor.SetValue(AsByteSpan(responseParams.metadataForRequestor)); } } @@ -218,8 +217,8 @@ const chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::ApplyUpdateRequest::DecodableType & commandData, MTROtaSoftwareUpdateProviderClusterApplyUpdateRequestParams * commandParams) { - commandParams.updateToken = [NSData dataWithBytes:commandData.updateToken.data() length:commandData.updateToken.size()]; - commandParams.newVersion = [NSNumber numberWithInt:commandData.newVersion]; + commandParams.updateToken = AsData(commandData.updateToken); + commandParams.newVersion = [NSNumber numberWithUnsignedLong:commandData.newVersion]; } void MTROTAProviderDelegateBridge::ConvertFromApplyUpdateRequestResponseParms( @@ -228,13 +227,13 @@ { response.action = static_cast([responseParams.action intValue]); - response.delayedActionTime = [responseParams.delayedActionTime intValue]; + response.delayedActionTime = [responseParams.delayedActionTime unsignedIntValue]; } void MTROTAProviderDelegateBridge::ConvertToNotifyUpdateAppliedParams( const chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::NotifyUpdateApplied::DecodableType & commandData, MTROtaSoftwareUpdateProviderClusterNotifyUpdateAppliedParams * commandParams) { - commandParams.updateToken = [NSData dataWithBytes:commandData.updateToken.data() length:commandData.updateToken.size()]; - commandParams.softwareVersion = [NSNumber numberWithInt:commandData.softwareVersion]; + commandParams.updateToken = AsData(commandData.updateToken); + commandParams.softwareVersion = [NSNumber numberWithUnsignedLong:commandData.softwareVersion]; }