MTRDevice should retry commands if it gets a BUSY response.

Since this can delay command invocation, accounts for that in the timed invoke case.
bzbarsky-apple · Oct 17, 2023 · d9c6c2f · d9c6c2f
1 parent d140d5c
commit d9c6c2f
Show file tree

Hide file tree

Showing 3 changed files with 44 additions and 7 deletions.
diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm
@@ -1112,6 +1112,11 @@ - (void)_invokeCommandWithEndpointID:(NSNumber *)endpointID
     serverSideProcessingTimeout = [serverSideProcessingTimeout copy];
     timeout = [timeout copy];
 
+    NSDate * cutoffTime;
+    if (timeout) {
+        cutoffTime = [NSDate dateWithTimeIntervalSinceNow:(timeout.doubleValue / 1000)];
+    }
+
     uint64_t expectedValueID = 0;
     NSMutableArray<MTRAttributePath *> * attributePaths = nil;
     if (expectedValues) {
@@ -1130,26 +1135,51 @@ - (void)_invokeCommandWithEndpointID:(NSNumber *)endpointID
         *stop = YES;
     }];
     [workItem setReadyHandler:^(MTRDevice * device, NSInteger retryCount, MTRAsyncWorkCompletionBlock workCompletion) {
+        auto workDone = ^(NSArray<NSDictionary<NSString *, id> *> * _Nullable values, NSError * _Nullable error) {
+            dispatch_async(queue, ^{
+                completion(values, error);
+            });
+            if (error && expectedValues) {
+                [self removeExpectedValuesForAttributePaths:attributePaths expectedValueID:expectedValueID];
+            }
+            workCompletion(MTRAsyncWorkComplete);
+        };
+
+        NSNumber * timedInvokeTimeout = nil;
+        if (timeout) {
+            auto * now = [NSDate now];
+            if ([now compare:cutoffTime] == NSOrderedDescending) {
+                // Our timed invoke timeout has expired already.  Command
+                // was queued for too long.  Do not send it out.
+                workDone(nil, [MTRError errorForIMStatusCode:Status::Timeout]);
+                return;
+            }
+
+            // Recompute the actual timeout left, accounting for time spent
+            // in our queuing and retries.
+            timedInvokeTimeout = @([cutoffTime timeIntervalSinceDate:now] * 1000);
+        }
         MTRBaseDevice * baseDevice = [self newBaseDevice];
         [baseDevice
             _invokeCommandWithEndpointID:endpointID
                                clusterID:clusterID
                                commandID:commandID
                            commandFields:commandFields
-                      timedInvokeTimeout:timeout
+                      timedInvokeTimeout:timedInvokeTimeout
              serverSideProcessingTimeout:serverSideProcessingTimeout
                                    queue:self.queue
                               completion:^(NSArray<NSDictionary<NSString *, id> *> * _Nullable values, NSError * _Nullable error) {
                                   // Log the data at the INFO level (not usually persisted permanently),
                                   // but make sure we log the work completion at the DEFAULT level.
                                   MTR_LOG_INFO("Invoke work item [%llu] received command response: %@ error: %@", workItemID, values, error);
-                                  dispatch_async(queue, ^{
-                                      completion(values, error);
-                                  });
-                                  if (error && expectedValues) {
-                                      [self removeExpectedValuesForAttributePaths:attributePaths expectedValueID:expectedValueID];
+                                  // TODO: This 5-retry cap is very arbitrary.
+                                  // TODO: Should there be some sort of backoff here?
+                                  if (error != nil && error.domain == MTRInteractionErrorDomain && error.code == MTRInteractionErrorCodeBusy && retryCount < 5) {
+                                      workCompletion(MTRAsyncWorkNeedsRetry);
+                                      return;
                                   }
-                                  workCompletion(MTRAsyncWorkComplete);
+
+                                  workDone(values, error);
                               }];
     }];
     [_asyncWorkQueue enqueueWorkItem:workItem descriptionWithFormat:@"invoke %@ %@ %@", endpointID, clusterID, commandID];

diff --git a/src/darwin/Framework/CHIP/MTRError.mm b/src/darwin/Framework/CHIP/MTRError.mm
@@ -232,6 +232,11 @@ + (NSError *)errorForIMStatus:(const chip::app::StatusIB &)status
     return [NSError errorWithDomain:MTRInteractionErrorDomain code:chip::to_underlying(status.mStatus) userInfo:userInfo];
 }
 
++ (NSError *)errorForIMStatusCode:(chip::Protocols::InteractionModel::Status)status
+{
+    return [self errorForIMStatus:chip::app::StatusIB(status)];
+}
+
 + (CHIP_ERROR)errorToCHIPErrorCode:(NSError * _Nullable)error
 {
     if (error == nil) {

diff --git a/src/darwin/Framework/CHIP/MTRError_Internal.h b/src/darwin/Framework/CHIP/MTRError_Internal.h
@@ -21,13 +21,15 @@
 
 #include <app/MessageDef/StatusIB.h>
 #include <lib/core/CHIPError.h>
+#include <protocols/interaction_model/StatusCode.h>
 
 NS_ASSUME_NONNULL_BEGIN
 
 MTR_HIDDEN
 @interface MTRError : NSObject
 + (NSError * _Nullable)errorForCHIPErrorCode:(CHIP_ERROR)errorCode;
 + (NSError * _Nullable)errorForIMStatus:(const chip::app::StatusIB &)status;
++ (NSError * _Nullable)errorForIMStatusCode:(chip::Protocols::InteractionModel::Status)status;
 + (CHIP_ERROR)errorToCHIPErrorCode:(NSError * _Nullable)error;
 @end