Skip to content

Commit 2186034

Browse files
nivi-applebzbarsky-apple
authored and
pull[bot]
committed
Clean up the code that sends the query image response from the provider (#24933)
* Clean up the code that sends the query image response from the provider * Update src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Address review comments * Update src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Add comments for why we do not reset state when we return busy from Configure State --------- Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
1 parent 6bcd876 commit 2186034

File tree

2 files changed

+67
-50
lines changed

2 files changed

+67
-50
lines changed

‎src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ class MTROTAProviderDelegateBridge : public chip::app::Clusters::OTAProviderDele
5353
static CHIP_ERROR ConvertToQueryImageParams(
5454
const chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::QueryImage::DecodableType & commandData,
5555
MTROTASoftwareUpdateProviderClusterQueryImageParams * commandParams);
56-
static void ConvertFromQueryImageResponseParms(
56+
static void ConvertFromQueryImageResponseParams(
5757
const MTROTASoftwareUpdateProviderClusterQueryImageResponseParams * responseParams,
5858
chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::QueryImageResponse::Type & response);
5959
static void ConvertToApplyUpdateRequestParams(

‎src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm

+66-49
Original file line numberDiff line numberDiff line change
@@ -461,9 +461,7 @@ CHIP_ERROR ConfigureState(chip::FabricIndex fabricIndex, chip::NodeId nodeId)
461461
CHIP_ERROR err = mSystemLayer->StartTimer(kBdxInitReceivedTimeout, HandleBdxInitReceivedTimeoutExpired, this);
462462
LogErrorOnFailure(err);
463463

464-
// The caller of this method maps CHIP_ERROR to specific BDX Status Codes (see GetBdxStatusCodeFromChipError)
465-
// and those are used by the BDX session to prepare the status report.
466-
VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_ERROR_INCORRECT_STATE);
464+
ReturnErrorOnFailure(err);
467465

468466
mFabricIndex.SetValue(fabricIndex);
469467
mNodeId.SetValue(nodeId);
@@ -615,7 +613,7 @@ bool GetPeerNodeInfo(CommandHandler * commandHandler, const ConcreteCommandPath
615613
auto * commandParams = [[MTROTASoftwareUpdateProviderClusterQueryImageParams alloc] init];
616614
CHIP_ERROR err = ConvertToQueryImageParams(commandData, commandParams);
617615
if (err != CHIP_NO_ERROR) {
618-
commandObj->AddStatus(commandPath, Protocols::InteractionModel::Status::InvalidCommand);
616+
commandObj->AddStatus(commandPath, StatusIB(err).mStatus);
619617
return;
620618
}
621619

@@ -635,64 +633,83 @@ bool GetPeerNodeInfo(CommandHandler * commandHandler, const ConcreteCommandPath
635633
ChipLogDetail(Controller, "QueryImage: application responded with: %s",
636634
[[data description] cStringUsingEncoding:NSUTF8StringEncoding]);
637635

638-
Commands::QueryImageResponse::Type response;
639-
ConvertFromQueryImageResponseParms(data, response);
640-
641636
auto hasUpdate = [data.status isEqual:@(MTROtaSoftwareUpdateProviderOTAQueryStatusUpdateAvailable)];
642637
auto isBDXProtocolSupported = [commandParams.protocolsSupported
643638
containsObject:@(MTROtaSoftwareUpdateProviderOTADownloadProtocolBDXSynchronous)];
644639

645-
if (hasUpdate && isBDXProtocolSupported) {
646-
auto fabricIndex = handler->GetSubjectDescriptor().fabricIndex;
647-
auto nodeId = handler->GetSubjectDescriptor().subject;
648-
CHIP_ERROR err = gOtaSender.PrepareForTransfer(fabricIndex, nodeId);
649-
if (CHIP_NO_ERROR != err) {
650-
LogErrorOnFailure(err);
651-
if (err == CHIP_ERROR_BUSY) {
652-
Commands::QueryImageResponse::Type busyResponse;
653-
busyResponse.status = static_cast<OTAQueryStatus>(MTROTASoftwareUpdateProviderOTAQueryStatusBusy);
654-
busyResponse.delayedActionTime.SetValue(response.delayedActionTime.ValueOr(kDelayedActionTimeSeconds));
655-
handler->AddResponse(cachedCommandPath, busyResponse);
656-
handle.Release();
657-
return;
658-
}
659-
handler->AddStatus(cachedCommandPath, Protocols::InteractionModel::Status::Failure);
660-
handle.Release();
661-
gOtaSender.ResetState();
662-
return;
663-
}
664-
auto targetNodeId
665-
= handler->GetExchangeContext()->GetSessionHandle()->AsSecureSession()->GetLocalScopedNodeId();
640+
// The logic we are following here is if none of the protocols supported by the requestor are supported by us, we
641+
// can't transfer the image even if we had an image available and we would return a Protocol Not Supported status.
642+
// Assumption here is the requestor would send us a list of all the protocols it supports. If one/more of the
643+
// protocols supported by the requestor are supported by us, we check if an image is not available due to various
644+
// reasons - image not available, delegate reporting busy, we will respond with the status in the delegate response.
645+
// If update is available, we try to prepare for transfer and build the uri in the response with a status of Image
646+
// Available
647+
648+
// If the protocol requested is not supported, return status - Protocol Not Supported
649+
if (!isBDXProtocolSupported) {
650+
Commands::QueryImageResponse::Type response;
651+
response.status
652+
= static_cast<OTAQueryStatus>(MTROTASoftwareUpdateProviderOTAQueryStatusDownloadProtocolNotSupported);
653+
handler->AddResponse(cachedCommandPath, response);
654+
handle.Release();
655+
return;
656+
}
666657

667-
char uriBuffer[kMaxBDXURILen];
668-
MutableCharSpan uri(uriBuffer);
669-
err = bdx::MakeURI(targetNodeId.GetNodeId(), AsCharSpan(data.imageURI), uri);
670-
if (CHIP_NO_ERROR != err) {
671-
LogErrorOnFailure(err);
672-
handler->AddStatus(cachedCommandPath, Protocols::InteractionModel::Status::Failure);
658+
Commands::QueryImageResponse::Type delegateResponse;
659+
ConvertFromQueryImageResponseParams(data, delegateResponse);
660+
661+
// If update is not available, return the delegate response
662+
if (!hasUpdate) {
663+
handler->AddResponse(cachedCommandPath, delegateResponse);
664+
handle.Release();
665+
return;
666+
}
667+
668+
// If there is an update available, try to prepare for a transfer.
669+
auto fabricIndex = handler->GetSubjectDescriptor().fabricIndex;
670+
auto nodeId = handler->GetSubjectDescriptor().subject;
671+
CHIP_ERROR err = gOtaSender.PrepareForTransfer(fabricIndex, nodeId);
672+
if (CHIP_NO_ERROR != err) {
673+
674+
// Handle busy error separately as we have a query image response status that maps to busy
675+
if (err == CHIP_ERROR_BUSY) {
676+
ChipLogError(
677+
Controller, "Responding with Busy due to being in the middle of handling another BDX transfer");
678+
Commands::QueryImageResponse::Type response;
679+
response.status = static_cast<OTAQueryStatus>(MTROTASoftwareUpdateProviderOTAQueryStatusBusy);
680+
response.delayedActionTime.SetValue(delegateResponse.delayedActionTime.ValueOr(kDelayedActionTimeSeconds));
681+
handler->AddResponse(cachedCommandPath, response);
673682
handle.Release();
674-
gOtaSender.ResetState();
683+
// We do not reset state when we get the busy error because that means we are locked in a BDX transfer
684+
// session with another requestor when we get this query image request. We do not want to interrupt the
685+
// ongoing transfer instead just respond to the second requestor with a busy status and a delayedActionTime
686+
// in which the requestor can retry.
675687
return;
676688
}
677-
678-
response.imageURI.SetValue(uri);
679-
handler->AddResponse(cachedCommandPath, response);
689+
LogErrorOnFailure(err);
690+
handler->AddStatus(cachedCommandPath, StatusIB(err).mStatus);
680691
handle.Release();
692+
// We need to reset state here to clean up any initialization we might have done including starting the BDX
693+
// timeout timer while preparing for transfer if any failure occurs afterwards.
694+
gOtaSender.ResetState();
681695
return;
682696
}
683-
if (!hasUpdate) {
684-
// Send whatever error response our delegate decided on.
685-
handler->AddResponse(cachedCommandPath, response);
686-
} else {
687-
// We must have isBDXProtocolSupported false. Send the corresponding error status.
688-
Commands::QueryImageResponse::Type protocolNotSupportedResponse;
689-
protocolNotSupportedResponse.status
690-
= static_cast<OTAQueryStatus>(MTROTASoftwareUpdateProviderOTAQueryStatusDownloadProtocolNotSupported);
691-
handler->AddResponse(cachedCommandPath, protocolNotSupportedResponse);
697+
auto targetNodeId = handler->GetExchangeContext()->GetSessionHandle()->AsSecureSession()->GetLocalScopedNodeId();
698+
699+
char uriBuffer[kMaxBDXURILen];
700+
MutableCharSpan uri(uriBuffer);
701+
err = bdx::MakeURI(targetNodeId.GetNodeId(), AsCharSpan(data.imageURI), uri);
702+
if (CHIP_NO_ERROR != err) {
703+
LogErrorOnFailure(err);
704+
handler->AddStatus(cachedCommandPath, StatusIB(err).mStatus);
705+
handle.Release();
706+
gOtaSender.ResetState();
707+
return;
692708
}
709+
delegateResponse.imageURI.SetValue(uri);
710+
handler->AddResponse(cachedCommandPath, delegateResponse);
693711
handle.Release();
694712
}
695-
696713
errorHandler:^(NSError *) {
697714
// Not much we can do here
698715
}];
@@ -867,7 +884,7 @@ bool GetPeerNodeInfo(CommandHandler * commandHandler, const ConcreteCommandPath
867884
return CHIP_NO_ERROR;
868885
}
869886

870-
void MTROTAProviderDelegateBridge::ConvertFromQueryImageResponseParms(
887+
void MTROTAProviderDelegateBridge::ConvertFromQueryImageResponseParams(
871888
const MTROTASoftwareUpdateProviderClusterQueryImageResponseParams * responseParams,
872889
Commands::QueryImageResponse::Type & response)
873890
{

0 commit comments

Comments
 (0)