Changed logic to only report once, and also improve documentation in comments

Author: jtung-apple

src/darwin/Framework/CHIP/MTRDevice.h

@@ -405,6 +405,10 @@ MTR_EXTERN NSString * const MTRDataVersionKey MTR_NEWLY_AVAILABLE;
 
 /**
  * Notifies delegate when the device attribute cache has been primed with initial configuration data of the device
+ *
+ * This is called when the MTRDevice object goes from not knowing the device to having cached the first attribute reports that include basic mandatory information, e.g. Descriptor clusters.
+ *
+ * The intention is that after this is called, the client should be able to call read for mandatory attributes and likely expect non-nil values.
  */
 - (void)deviceCachePrimed:(MTRDevice *)device MTR_NEWLY_AVAILABLE;
 

src/darwin/Framework/CHIP/MTRDevice.mm

@@ -503,13 +503,15 @@ - (void)setDelegate:(id<MTRDeviceDelegate>)delegate queue:(dispatch_queue_t)queu
     _weakDelegate = [MTRWeakReference weakReferenceWithObject:delegate];
     _delegateQueue = queue;
 
+    // If Check if cache is already primed and client hasn't been informed yet, call the -deviceCachePrimed: callback
+    if (!_delegateDeviceCachePrimedCalled && [self _isCachePrimedWithInitialConfigurationData]) {
+        [self _callDelegateDeviceCachePrimed];
+    }
+
     if (setUpSubscription) {
         [self _setupSubscription];
     }
 
-    // Check if cache is already primed from storage
-    [self _checkIfCacheIsPrimed];
-
     os_unfair_lock_unlock(&self->_lock);
 }
 
@@ -638,6 +640,11 @@ - (void)_handleSubscriptionEstablished
     // reset subscription attempt wait time when subscription succeeds
     _lastSubscriptionAttemptWait = 0;
 
+    // As subscription is established, check if the delegate needs to be informed
+    if (!_delegateDeviceCachePrimedCalled) {
+        [self _callDelegateDeviceCachePrimed];
+    }
+
     [self _changeState:MTRDeviceStateReachable];
 
     os_unfair_lock_unlock(&self->_lock);
@@ -769,11 +776,6 @@ - (void)_handleReportEnd
     _receivingPrimingReport = NO;
     _estimatedStartTimeFromGeneralDiagnosticsUpTime = nil;
 
-    // First subscription report is priming report
-    if (!_delegateDeviceCachePrimedCalled) {
-        [self _callDelegateDeviceCachePrimed];
-    }
-
 // For unit testing only
 #ifdef DEBUG
     id delegate = _weakDelegate.strongObject;
@@ -1993,17 +1995,24 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
 
 - (void)setAttributeValues:(NSArray<NSDictionary *> *)attributeValues reportChanges:(BOOL)reportChanges
 {
+    os_unfair_lock_lock(&self->_lock);
+
     if (reportChanges) {
-        [self _handleAttributeReport:attributeValues];
+        [self _reportAttributes:[self _getAttributesToReportWithReportedValues:attributeValues]];
     } else {
-        os_unfair_lock_lock(&self->_lock);
         for (NSDictionary * responseValue in attributeValues) {
             MTRAttributePath * path = responseValue[MTRAttributePathKey];
             NSDictionary * dataValue = responseValue[MTRDataKey];
             _readCache[path] = dataValue;
         }
-        os_unfair_lock_unlock(&self->_lock);
     }
+
+    // If cache is set from storage and is primed with initial configuration data, then assume the client had beeen informed in the past, and mark that the callback has been called
+    if ([self _isCachePrimedWithInitialConfigurationData]) {
+        _delegateDeviceCachePrimedCalled = YES;
+    }
+
+    os_unfair_lock_unlock(&self->_lock);
 }
 
 // If value is non-nil, associate with expectedValueID
@@ -2181,25 +2190,19 @@ - (void)_removeExpectedValueForAttributePath:(MTRAttributePath *)attributePath e
 }
 
 // This method checks if there is a need to inform delegate that the attribute cache has been "primed"
-//   - The delegate callback is only called once
-- (void)_checkIfCacheIsPrimed
+- (BOOL)_isCachePrimedWithInitialConfigurationData
 {
     os_unfair_lock_assert_owner(&self->_lock);
 
-    // Only send the callback once per lifetime of MTRDevice
-    if (_delegateDeviceCachePrimedCalled) {
-        return;
-    }
-
     // Check if root node descriptor exists
     NSDictionary * rootDescriptorPartsListDataValue = _readCache[[MTRAttributePath attributePathWithEndpointID:@(0) clusterID:@(MTRClusterIDTypeDescriptorID) attributeID:@(MTRAttributeIDTypeClusterDescriptorAttributePartsListID)]];
     if (!rootDescriptorPartsListDataValue || ![MTRArrayValueType isEqualToString:rootDescriptorPartsListDataValue[MTRTypeKey]]) {
-        return;
+        return NO;
     }
     NSArray * partsList = rootDescriptorPartsListDataValue[MTRValueKey];
     if (![partsList isKindOfClass:[NSArray class]] || !partsList.count) {
         MTR_LOG_ERROR("%@ unexpected type %@ for parts list %@", self, [partsList class], partsList);
-        return;
+        return NO;
     }
 
     // Check if we have cached descriptor clusters for each listed endpoint
@@ -2215,11 +2218,11 @@ - (void)_checkIfCacheIsPrimed
         }
         NSDictionary * descriptorDeviceTypeListDataValue = _readCache[[MTRAttributePath attributePathWithEndpointID:endpoint clusterID:@(MTRClusterIDTypeDescriptorID) attributeID:@(MTRAttributeIDTypeClusterDescriptorAttributeDeviceTypeListID)]];
         if (![MTRArrayValueType isEqualToString:descriptorDeviceTypeListDataValue[MTRTypeKey]] || !descriptorDeviceTypeListDataValue[MTRValueKey]) {
-            return;
+            return NO;
         }
     }
 
-    [self _callDelegateDeviceCachePrimed];
+    return YES;
 }
 
 - (MTRBaseDevice *)newBaseDevice

src/darwin/Framework/CHIPTests/MTRDeviceTests.m

@@ -2878,18 +2878,21 @@ - (void)test031_MTRDeviceAttributeCacheLocalTestStorage
     device = [MTRDevice deviceWithNodeID:@(kDeviceId) controller:sController];
 
     XCTestExpectation * resubGotReportsExpectation = [self expectationWithDescription:@"Attribute and Event reports have been received for resubscription"];
-    XCTestExpectation * gotDeviceCachePrimedAgain = [self expectationWithDescription:@"Device cache primed upon load from persistence"];
     delegate.onReportEnd = ^{
         [resubGotReportsExpectation fulfill];
         __strong __auto_type strongDelegate = weakDelegate;
         strongDelegate.onReportEnd = nil;
     };
+    __block BOOL onDeviceCachePrimedCalled = NO;
     delegate.onDeviceCachePrimed = ^{
-        [gotDeviceCachePrimedAgain fulfill];
+        onDeviceCachePrimedCalled = YES;
     };
     [device setDelegate:delegate queue:queue];
 
-    [self waitForExpectations:@[ gotDeviceCachePrimedAgain, resubGotReportsExpectation ] timeout:60];
+    [self waitForExpectations:@[ resubGotReportsExpectation ] timeout:60];
+
+    // Make sure that the new callback is only ever called once, the first time subscription was primed
+    XCTAssertFalse(onDeviceCachePrimedCalled);
 
     NSUInteger attributesReportedWithSecondSubscription = [device unitTestAttributesReportedSinceLastCheck];